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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, May 24, 2024 at 12:16:00PM +0100, Alejandro Vallejo wrote:
> 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.

Oh, indeed, hvm_get_entry() uses strict checking and will refuse to
return the entry if sizes don't match.  There seems to be no way to
avoid the copy if we want to do this in a sane way.

Thanks, Roger.