Re: [Xen-devel] IOREQ server on Arm

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@xxxxxxx]
> Sent: 27 September 2018 10:42
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; 'Jan Beulich'
> <JBeulich@xxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: IOREQ server on Arm
> 
> Hi Paul,
> 
> On 09/27/2018 09:38 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@xxxxxxx]
> >> Sent: 26 September 2018 22:32
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; 'Jan Beulich'
> >> <JBeulich@xxxxxxxx>
> >> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne
> >> <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> xen-
> >> devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> >> Subject: Re: IOREQ server on Arm
> >>
> >> Hi Paul,
> >>
> >> On 09/26/2018 01:01 PM, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >>>> Sent: 26 September 2018 12:57
> >>>> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> >>>> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> >>>> <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>;
> >>>> Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel <xen-
> >>>> devel@xxxxxxxxxxxxxxxxxxxx>
> >>>> Subject: RE: IOREQ server on Arm
> >>>>
> >>>>>>> On 26.09.18 at 13:02, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>>>> --- a/xen/common/memory.c
> >>>>> +++ b/xen/common/memory.c
> >>>>> @@ -1105,8 +1105,11 @@ static int acquire_resource(
> >>>>>
> >>>>>            for ( i = 0; !rc && i < xmar.nr_frames; i++ )
> >>>>>            {
> >>>>> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> >>>>> -                                       _mfn(mfn_list[i]));
> >>>>> +            rc = (xmar.flags & XENMEM_rsrc_acq_caller_owned) ?
> >>>>> +                guest_physmap_add_entry(currd, gfn_list[i],
> >>>>> +                                        _mfn(mfn_list[i]), 0,
> >>>> p2m_ram_rw) :
> >>>>> +                set_foreign_p2m_entry(currd, gfn_list[i],
> >>>>> +                                      _mfn(mfn_list[i]));
> >>>>>                /* rc should be -EIO for any iteration other than the
> >> first
> >>>> */
> >>>>>                if ( rc && i )
> >>>>>                    rc = -EIO;
> >>>>>
> >>>>> But the guest_physmap_add_entry() is problematic as it will IOMMU
> map
> >>>> pages
> >>>>> as well, which is probably not wanted.
> >>>>
> >>>> Yeah, I'd prefer if we avoided establishing IOMMU mappings here.
> >>>> How about transforming set_foreign_p2m_entry() into
> >>>> set_special_p2m_entry(), with a type passed in?
> >>>>
> >>>
> >>> That sounds like it might work.
> >>>
> >>> Julien, do you want page types to distinguish caller-owned resources
> >> from normal RAM are you ok with p2m_ram_rw even though it could be
> subject
> >> of another domain's foreign map?
> >>
> >> Based on your previous e-mail, I would be fine with that on Arm.
> >>
> >> This brings me to the next question. Do you expect
> set_special_p2m_entry
> >> to take a reference on the page?
> >>
> >> If not, we may run into some troubles because AFAICT you can map twice
> >> the ioreq page in a guest but reference will only be taken on the
> >> allocation.
> >>
> >> However, the unmap path will always drop a reference when removing the
> >> page. This is because Xen at the moment, reference will not be taken on
> >> mapping but allocation (we assume a page could not be mapped twice in a
> >> guest).
> >>
> >> Foreign mapping on Arm are a bit special because we get a reference on
> >> mapping them and will drop it when the mapping disappear. So we would
> >> not have any problem there.
> >>
> >> Any thoughts?
> >
> > Well, as Jan says, on x86 we don't reference count in the P2M so
> multiple mappings should not be an issue AFAICT.
> 
> I understand that you don't have reference count in the P2M (that's the
> same on Arm today except for foreign mapping). But I think I can list at
> least 2 major issues with the design today. Let me give an example based
> on my understanding.
> 
>    1. DM requests to map the IOREQ page
>       a) page allocated (one reference)
>       b) get reference (will be dropped when the IOREQ server is
> destroyed)
> 
>    2. DM requests to map the IOREQ page (second time)
>       No reference taken
> 
>    3. DM unmap the IOREQ page
>    4. DM unmap the IOREQ page
> 
> AFAIU, 3. 4. would be done through XENMEM_remove_from_physmap. So no
> reference dropped there. While the reference 1.b) will be dropped in
> hvm_free_ioreq_mfn. AFAICT 1.a) would be kept until the domain die. This
> would result to Xen memory exhaustion in long term. Did I miss anything?
> 

1.a) would be kept until the IOREQ server is destroyed, which will happen 
either at domain destruction *or* when the DM destroys it.

> But, I think there are another way for badly written guest to remove the
> page. It looks like you can use XENMEM_decrease_reservation as the page
> belongs to the guest. So a reference would be dropped by 3. and 4.
> 

How so? The pages don't belong to the guest; they belong the the DM domain. 
They never appear in guest P2M so how can the guest decrease_reservation them? 
Are you worried about the DM domain doing a decrease reservation?

  Paul

> While 3. will drop the reference drop by 1.a), 4. may drop the reference
> from 1.b) and releasing the page for good. Although the page will still
> be associated with the IOREQ server until it has been effectively
> destroyed. Did I miss anything in the code?
> 
> > It sounds like resource mapping should be treated the same way as
> foreign mapping (albeit with a non-foreign domid) such that the reference
> acquisition occurs at map time.
> 
> If my understanding is correct then yes it would be much safer to get
> reference here.
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel