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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.10.17 at 16:45, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 09 October 2017 16:21
>> >>> On 06.10.17 at 14:25, <paul.durrant@xxxxxxxxxx> wrote:
>> > @@ -784,6 +885,45 @@ 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 int 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;
>> > +
>> > +    if ( idx == 0 )
>> 
>> switch() ?
> 
> Yes, but if idx can exceed 1 in future (which would need to be the case to 
> support greater numbers of vcpus) then I guess it may change back.

If you anticipate that to not be expressable by case labels (not even
by range ones), then perhaps indeed better stick to if/else.

>> > @@ -3866,6 +3867,27 @@ int xenmem_add_to_physmap_one(
>> >      return rc;
>> >  }
>> >
>> > +int xenmem_acquire_ioreq_server(struct domain *d, unsigned int id,
>> > +                                unsigned long frame,
>> > +                                unsigned long nr_frames,
>> > +                                unsigned long mfn_list[])
>> > +{
>> > +    unsigned int i;
>> > +
>> > +    for ( i = 0; i < nr_frames; i++ )
>> > +    {
>> > +        mfn_t mfn;
>> > +        int rc = hvm_get_ioreq_server_frame(d, id, frame + i, &mfn);
>> 
>> Coming back to the question of the size of the "frame" interface
>> structure field, note how you silently truncate the upper 32 bits
>> here.
> 
> OK. For this resource type I can't see 64-bits being needed, but I'll carry 
> them through.

Carrying the full value through is one option. The other is to bail
early when finding the upper bits set.

>> > --- a/xen/include/public/memory.h
>> > +++ b/xen/include/public/memory.h
>> > @@ -609,15 +609,26 @@ struct xen_mem_acquire_resource {
>> >      domid_t domid;
>> >      /* IN - the type of resource */
>> >      uint16_t type;
>> > +
>> > +#define XENMEM_resource_ioreq_server 0
>> > +
>> >      /*
>> >       * IN - a type-specific resource identifier, which must be zero
>> >       *      unless stated otherwise.
>> > +     *
>> > +     * type == XENMEM_resource_ioreq_server -> id == ioreq server id
>> >       */
>> >      uint32_t id;
>> >      /* IN - number of (4K) frames of the resource to be mapped */
>> >      uint32_t nr_frames;
>> >      uint32_t pad;
>> > -    /* IN - the index of the initial frame to be mapped */
>> > +    /* IN - the index of the initial frame to be mapped
>> > +     *
>> > +     * type == XENMEM_resource_ioreq_server -> frame == 0 -> bufioreq
>> > +     *                                                       page
>> > +     *                                         frame == 1 -> ioreq
>> > +     *                                                       page
>> > +     */
>> 
>> Long comment or not I think you want to introduce constants
>> for these two numbers.
>> 
> 
> Yes, that would probably be better although increasing the number of 
> supported vcpus may mean that >1 becomes valid in future.

Sure, but that would then still better be expressed by a suitable
macro (perhaps one long the lines of MSR_P6_PERFCTR()).

Jan


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