linux-kernel - Re: [PART2 PATCH v5 10/12] svm: Introduces AVIC per-VM ID

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 18 Aug 2016 19:24:33 +0700
From:	Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
To:	Radim Krčmář <rkrcmar@...hat.com>
CC:	<joro@...tes.org>, <pbonzini@...hat.com>,
	<alex.williamson@...hat.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <sherry.hurwitz@....com>
Subject: Re: [PART2 PATCH v5 10/12] svm: Introduces AVIC per-VM ID

Hi Radim,

On 8/12/16 21:16, Radim Krčmář wrote:
>> +static DEFINE_SPINLOCK(avic_vm_id_lock);
>> > +
>> >  static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
>> >  static void svm_flush_tlb(struct kvm_vcpu *vcpu);
>> >  static void svm_complete_interrupts(struct vcpu_svm *svm);
>> > @@ -1280,10 +1296,61 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>> >  	return 0;
>> >  }
>> >
>> > +static inline int avic_vm_id_init(void)
>> > +{
>> > +	if (avic_vm_id_bm)
>> > +		return 0;
>> > +
>> > +	avic_vm_id_bm = kcalloc(BITS_TO_LONGS(AVIC_VM_ID_MASK),
> Allocation is off by one.  avic_get_next_vm_id() uses
>   if (id <= AVIC_VM_ID_MASK)
>     __set_bit(id, avic_vm_id_bm);
>
> and id=AVIC_VM_ID_MASK is stored in the AVIC_VM_ID_MASK+1 th bit.

Ah... right. Sorry :(

>> > +static inline int avic_get_next_vm_id(void)
>> > +{
>> > +	int id;
>> > +
>> > +	spin_lock(&avic_vm_id_lock);
>> > +
>> > +	/* AVIC VM ID is one-based. */
> Why?

I use VM-ID 0 to represent unassigned ID since we use it to encode 
ga_tag, and ga_tag=0 out of reset by hardware.

>> > +	id = find_next_zero_bit(avic_vm_id_bm, 1, 1);
> The second argument is size, so this should always return 1. :)
>

My bad. I'll change to (AVIC_VM_ID_MASK + 1).

>> > +	if (id <= AVIC_VM_ID_MASK)
>> > +		__set_bit(id, avic_vm_id_bm);
>> > +	else
>> > +		id = -EINVAL;
> It is not really a problem that can be handled with changing the values,
> so a temporary error would be nicer ... ENOMEM could be confusing and
> EAGAIN lead to a loop, but I still like them better.
>

Ok. I think EAGAIN is better in this case.

>> >  static int __init svm_init(void)
>> >  {
>> > +	int ret;
>> > +
>> > +	ret = avic_vm_id_init();
> This is certainly useless when the CPU doesn't have AVIC, so we could
> make it conditional.
>
> I would prefer to make the bitmap allocated at module load, though:
>
>   static DECLARE_BITMAP(avic_vm_id_bm, AVIC_VM_ID_MASK + 1);
>
> The size is 2 KiB with 24 bit AVIC_VM_ID_MASK, which is IMO much better
> than having extra lines of code dealing with allocation and failures.
>

I also prefer this suggestion.

Thanks again,
Suravee