Re: [Xen-devel] [PATCH v11 06/11] x86/hvm/ioreq: add a new mappable resource type...

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.10.17 at 16:17, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 16 October 2017 15:07
>> >>> On 12.10.17 at 18:25, <paul.durrant@xxxxxxxxxx> wrote:
>> > ... XENMEM_resource_ioreq_server
>> >
>> > This patch adds support for a new resource type that can be mapped using
>> > the XENMEM_acquire_resource memory op.
>> >
>> > If an emulator makes use of this resource type then, instead of mapping
>> > gfns, the IOREQ server will allocate pages from the heap. These pages
>> > will never be present in the P2M of the guest at any point and so are
>> > not vulnerable to any direct attack by the guest. They are only ever
>> > accessible by Xen and any domain that has mapping privilege over the
>> > guest (which may or may not be limited to the domain running the
>> emulator).
>> >
>> > NOTE: Use of the new resource type is not compatible with use of
>> >       XEN_DMOP_get_ioreq_server_info unless the XEN_DMOP_no_gfns
>> flag is
>> >       set.
>> >
>> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
>> > Acked-by: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>> > Reviewed-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>> 
>> Can you have validly retained this?
> 
> I didn't think the structure of this particular patch had changed that 
> fundamentally.

The structure didn't change that much, yes, but the page type
ref acquiring which you now do alter behavior meaningfully.

>> > @@ -777,6 +886,51 @@ int hvm_get_ioreq_server_info(struct domain *d,
>> ioservid_t id,
>> >      return rc;
>> >  }
>> >
>> > +int hvm_get_ioreq_server_frame(struct domain *d, ioservid_t id,
>> > +                               unsigned long idx, mfn_t *mfn)
>> > +{
>> > +    struct hvm_ioreq_server *s;
>> > +    int rc;
>> > +
>> > +    spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
>> > +
>> > +    if ( id == DEFAULT_IOSERVID )
>> > +        return -EOPNOTSUPP;
>> > +
>> > +    s = get_ioreq_server(d, id);
>> > +
>> > +    ASSERT(!IS_DEFAULT(s));
>> > +
>> > +    rc = hvm_ioreq_server_alloc_pages(s);
>> > +    if ( rc )
>> > +        goto out;
>> > +
>> > +    switch ( idx )
>> > +    {
>> > +    case XENMEM_resource_ioreq_server_frame_bufioreq:
>> > +        rc = -ENOENT;
>> > +        if ( !HANDLE_BUFIOREQ(s) )
>> > +            goto out;
>> > +
>> > +        *mfn = _mfn(page_to_mfn(s->bufioreq.page));
>> > +        rc = 0;
>> > +        break;
>> 
>> How about
>> 
>>         if ( HANDLE_BUFIOREQ(s) )
>>             *mfn = _mfn(page_to_mfn(s->bufioreq.page));
>>         else
>>             rc = -ENOENT;
>>         break;
>> 
> 
> Looking at the overall structure I prefer it as it is. If I could have got 
> rid of the out label by doing this then it might have been worth the change.

Okay, you're the maintainer. Just to clarify - what I find particularly
odd is the setting of rc to zero above, yet the other case block
relying on it already being zero when entering the switch().

>> > @@ -629,6 +634,10 @@ struct xen_mem_acquire_resource {
>> >       *      is optional if nr_frames is 0.
>> >       */
>> >      uint64_aligned_t frame;
>> > +
>> > +#define XENMEM_resource_ioreq_server_frame_bufioreq 0
>> > +#define XENMEM_resource_ioreq_server_frame_ioreq(n_) (1 + (n_))
>> 
>> I don't see what you need the trailing underscore for. This is
>> normally only needed on local variables defined in (gcc extended)
>> macros, which we generally can't use in a public header anyway.
> 
> I thought it was generally desirable to attempt to distinguish macro 
> arguments from variable to avoid name clashes. What do you prefer I should do 
> in a public header?

There are various cases to be considered here, but in the one
here there is no risk of name clash at all: Regardless of the
name of the parameter, any instance of it will be expanded
exactly once. Even if the expansion matches exactly the
parameter name, no issue will arise. There are certainly forms
of macros where some care is needed in how to name the
parameters. Trailing underscores to disambiguate names,
however, should - as said - rarely if ever be needed for other
than local variables inside the macro body (because _then_
there indeed can be name conflicts with outer scope variables).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel