Re: [PATCH v2 3/8] x86/vlapic: Move lapic_load_hidden migration checks to the check hook

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 23/05/2024 15:50, Roger Pau Monné wrote:
> On Wed, May 08, 2024 at 01:39:22PM +0100, Alejandro Vallejo wrote:
>> While at it, add a check for the reserved field in the hidden save area.
>>
>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
>> ---
>> v2:
>>   * New patch. Addresses the missing check for rsvd_zero in v1.
> 
> Oh, it would be better if this was done at the time when rsvd_zero is
> introduced.  I think this should be moved ahead of the series, so that
> the patch that introduces rsvd_zero can add the check in
> lapic_check_hidden().

I'll give that a whirl.

> 
>> ---
>>  xen/arch/x86/hvm/vlapic.c | 41 ++++++++++++++++++++++++++++-----------
>>  1 file changed, 30 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>> index 8a244100009c..2f06bff1b2cc 100644
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -1573,35 +1573,54 @@ static void lapic_load_fixup(struct vlapic *vlapic)
>>                 v, vlapic->loaded.id, vlapic->loaded.ldr, good_ldr);
>>  }
>>  
>> -static int cf_check lapic_load_hidden(struct domain *d, 
>> hvm_domain_context_t *h)
>> +static int cf_check lapic_check_hidden(const struct domain *d,
>> +                                       hvm_domain_context_t *h)
>>  {
>>      unsigned int vcpuid = hvm_load_instance(h);
>> -    struct vcpu *v;
>> -    struct vlapic *s;
>> +    struct hvm_hw_lapic s;
>>  
>>      if ( !has_vlapic(d) )
>>          return -ENODEV;
>>  
>>      /* Which vlapic to load? */
>> -    if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
>> +    if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL )
>>      {
>>          dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n",
>>                  d->domain_id, vcpuid);
>>          return -EINVAL;
>>      }
>> -    s = vcpu_vlapic(v);
>>  
>> -    if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 )
>> +    if ( hvm_load_entry_zeroextend(LAPIC, h, &s) )
> 
> Can't you use hvm_get_entry() to perform the sanity checks:
> 
> const struct hvm_hw_lapic *s = hvm_get_entry(LAPIC, h);
> 
> Thanks, Roger.

I don't think I can. Because the last field (rsvd_zero) might or might
not be there, so it needs to be zero-extended. Unless I misunderstood
what hvm_get_entry() is meant to do. It seems to check for exact sizes.

Cheers,
Alejandro