Re: [Xen-devel] [PATCH 03/10 v2] xen/arm: vpl011: Enable pl011 emulation for a guest domain in Xen

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Jan,

>> @@ -631,6 +632,9 @@ int arch_domain_create(struct domain *d, unsigned int 
>> domcr_flags,
>>      if ( (rc = domain_vtimer_init(d, config)) != 0 )
>>          goto fail;
>>
>> +    if ( domcr_flags & DOMCRF_vuart )
>> +        if ( (rc = domain_vpl011_init(d, config)) != 0 )
>> +            goto fail;
>>      update_domain_wallclock_time(d);
>
I am planning to remove the usage of domain creation flag to check
whether vuart is enabled/disabled. Please see my next comment. With
that change, domain_vpl011_init() will be called always. The
domain_vpl011_init() will check whether vuart is enabled or disabled
in the config structure passed. If vuart is enabled then it will go
ahead with vpl011 initialization else it will return without
initializing vpl011.

> This is ARM code, so my opinion may not mean much, but why two
> if()s instead of one, and why no blank line after your addition?
>
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -63,6 +63,8 @@ struct xen_domctl_createdomain {
>>   /* Is this a xenstore domain? */
>>  #define _XEN_DOMCTL_CDF_xs_domain     4
>>  #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
>> +#define _XEN_DOMCTL_VUART_enable      6
>> +#define XEN_DOMCTL_VUART_enable       (1U<<_XEN_DOMCTL_VUART_enable)
>
> As expressed before, I object to this addition, as doing things this
> way does not scale. I don't think I've seen any proper reply to my
> previous objection, and the patch description here certainly does
> not justify why an exception should be made in this case. As a
> side note, why bit 6 instead of the first available one (5)?
>
Sorry I missed to address this review comment. I will pass the vuart
enable/disable information through the configuration structure
xc_domain_configuration_t, which is passed transparently to
domain_vpl011_init() by the tool stack. With that change, we need not
use domain creation flags.

Regards,
Bhupinder

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