RE: [PATCH v2 04/11] xen/memory: Fix acquire_resource size semantics

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Sent: 24 September 2020 11:58
> To: paul@xxxxxxx; 'Xen-devel' <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: 'George Dunlap' <George.Dunlap@xxxxxxxxxxxxx>; 'Ian Jackson' 
> <iwj@xxxxxxxxxxxxxx>; 'Jan Beulich'
> <JBeulich@xxxxxxxx>; 'Stefano Stabellini' <sstabellini@xxxxxxxxxx>; 'Wei Liu' 
> <wl@xxxxxxx>; 'Julien
> Grall' <julien@xxxxxxx>; 'Michał Leszczyński' <michal.leszczynski@xxxxxxx>; 
> 'Hubert Jasudowicz'
> <hubert.jasudowicz@xxxxxxx>; 'Tamas K Lengyel' <tamas@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v2 04/11] xen/memory: Fix acquire_resource size semantics
> 
> On 24/09/2020 11:06, Paul Durrant wrote:
> >> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> >> index d1cfc8fb4a..e82307bdae 100644
> >> --- a/xen/arch/x86/mm.c
> >> +++ b/xen/arch/x86/mm.c
> >> @@ -4591,6 +4591,26 @@ int xenmem_add_to_physmap_one(
> >>      return rc;
> >>  }
> >>
> >> +unsigned int arch_resource_max_frames(
> >> +    struct domain *d, unsigned int type, unsigned int id)
> >> +{
> >> +    unsigned int nr = 0;
> >> +
> >> +    switch ( type )
> >> +    {
> >> +#ifdef CONFIG_HVM
> >> +    case XENMEM_resource_ioreq_server:
> >> +        if ( !is_hvm_domain(d) )
> >> +            break;
> >> +        /* One frame for the buf-ioreq ring, and one frame per 128 vcpus. 
> >> */
> >> +        nr = 1 + DIV_ROUND_UP(d->max_vcpus * sizeof(struct ioreq), 
> >> PAGE_SIZE);
> > The buf-ioreq ring is optional
> 
> Yes, but it's position within the resource, and effect on the position
> of the ioreq page(s), is not.

Oh yes, I was forgetting that this is fixed so...

Reviewed-by: Paul Durrant <paul@xxxxxxx>

> 
> >  so a caller using this value may still get a resource acquisition failure 
> > unless the id is used to
> actually look up and check the ioreq server in question for the actual 
> maximum.
> 
> Yes, but that is potentially true of *any* acquisition attempt, even if
> the id matches, because maybe someone else has destroyed the ioreq
> server, or the domain, in the meantime.
> 
> 
> What we have is an mmap() where the caller needs to know to not try and
> map page 0 for an ioreq server where buf-ioreq doesn't exist.
> 
> This is a direct consequence of:
> 
> #define XENMEM_resource_ioreq_server_frame_bufioreq 0
> #define XENMEM_resource_ioreq_server_frame_ioreq(n) (1 + (n))
> 
> and in practice, what a qemu/demu/other needs to do to map just the
> ioreq frames (in a manner compatible with >127 vcpu HVM domains) is to
> query the resource size and map size-1 pages from offset 1.

Yes.

  Paul

> 
> ~Andrew