Re: [PATCH v2 13/21] xen/dts: Add hypercalls to retrieve device node information

[Julien Grall <julien.grall@linaro.org>]

Hi Jan,

On 08/01/2014 09:50 AM, Jan Beulich wrote:
>>>> On 31.07.14 at 17:00, <julien.grall@linaro.org> wrote:
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -315,6 +315,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>      case XEN_DOMCTL_createdomain:
>>      case XEN_DOMCTL_getdomaininfo:
>>      case XEN_DOMCTL_test_assign_device:
>> +    case XEN_DOMCTL_dtdev_op:
>>          d = NULL;
>>          break;
>>      default:
>> @@ -1017,6 +1018,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>      }
>>      break;
>>  
>> +#ifdef HAS_DEVICE_TREE
>> +    case XEN_DOMCTL_dtdev_op:
>> +    {
>> +        ret = dt_do_domctl(op);
>> +        copyback = 1;
>> +    }
>> +    break;
>> +#endif
> 
> No pointless braces please.

Will drop it.


>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -946,6 +946,44 @@ typedef struct xen_domctl_vcpu_msrs xen_domctl_vcpu_msrs_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_vcpu_msrs_t);
>>  #endif
>>  
>> +/* Device Tree: Retrieve informations about a device node */
>> +struct xen_domctl_dtdev_op {
>> +    /* IN */
>> +    uint32_t plen;                  /* Length of the path */
>> +    XEN_GUEST_HANDLE(char) path;    /* Path to the device tree node */
> 
> XEN_GUEST_HANDLE_64?

Right. I never know when I should use XEN_GUEST_HANDLE_64 or
XEN_GUEST_HANDLE.

FYI, it's exactly the same on ARM.

> And padding between the two above
> members, or fields re-ordered?

I can reorder the field.

>> +#define DOMCTL_DTDEV_GET_INFO        0
>> +#define DOMCTL_DTDEV_GET_IRQ         1
>> +#define DOMCTL_DTDEV_GET_MMIO        2
>> +#define DOMCTL_DTDEV_GET_COMPAT      3
>> +    uint8_t op;
>> +    uint32_t pad0:24;
> 
> uint8_t pad0[3].

Ok.

>> +    uint32_t index;                 /* Index for the IRQ/MMIO to retrieve */
>> +    /* OUT */
>> +    union {
>> +        struct {
>> +            uint32_t num_irqs;      /* Number of IRQs */
>> +            uint32_t num_mmios;     /* Number of MMIOs */
>> +            uint32_t compat_len;    /* Length of the compatible string */
>> +        } info;
>> +        struct {
>> +            /* TODO: Do we need to handle MSI-X? */
>> +            uint32_t irq;           /* IRQ number */
>> +            /* TODO: Describe with defines the IRQ type */
> 
> ???

For now we are using the DT_IRQ_TYPE_* provided by the device tree (see
include/xen/device_tree.h). I haven't yet introduced specific IRQ type here.

> Also, are you planning to address these two TODOs before this gets
> ready to be committed?

I plan to handle the latter TODO (i.e IRQ type), for the MSI I don't
know how they will be described in the device tree. So I will drop the
TODO. It will be fine because it's a DOMCTL so it's possible to change
the interface easily.

>> +            uint32_t type;          /* IRQ type (i.e edge, level...) */
> 
> #define-s to specify what values here mean?

See my answer a bit above.

>> +        } irq;
>> +        struct {
>> +            xen_pfn_t mfn;
>> +            xen_pfn_t nr_mfn;
>> +        } mmio;
>> +        struct {
>> +            uint32_t len;           /* IN: Size of buffer. OUT: Size copied */
>> +            XEN_GUEST_HANDLE_64(char) compat;
> 
> Padding again?

I will invert the 2 fields.

Regards,

-- 
Julien Grall