Re: [Xen-devel] IOREQ server on Arm

[Julien Grall <julien.grall@xxxxxxx>]

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?
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.
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