[PATCH 1/2] rangeset: add RANGESETF_no_print flag

[PATCH 1/2] rangeset: add RANGESETF_no_print flag

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

There are range sets which should not be printed, so introduce a flag
which allows marking those as such. Implement relevant logic to skip
such entries while printing.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 xen/common/rangeset.c      | 3 +++
 xen/include/xen/rangeset.h | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index 885b6b15c229..939883a1d145 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -575,6 +575,9 @@ void rangeset_domain_printk(
 
     list_for_each_entry ( r, &d->rangesets, rangeset_list )
     {
+        if ( r->flags & RANGESETF_no_print )
+            continue;
+
         printk("    ");
         rangeset_printk(r);
         printk("\n");
diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 135f33f6066f..543540a88b6f 100644
--- a/xen/include/xen/rangeset.h
+++ b/xen/include/xen/rangeset.h
@@ -51,6 +51,9 @@ void rangeset_limit(
  /* Pretty-print range limits in hexadecimal. */
 #define _RANGESETF_prettyprint_hex 0
 #define RANGESETF_prettyprint_hex  (1U << _RANGESETF_prettyprint_hex)
+/* Do not print entries marked with this flag. */
+#define _RANGESETF_no_print 1
+#define RANGESETF_no_print  (1U << _RANGESETF_no_print)
 
 bool_t __must_check rangeset_is_empty(
     const struct rangeset *r);
-- 
2.25.1

[PATCH 2/2] vpci: use named rangeset for BARs

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Use a named range set instead of an anonymous one, but do not print it
while dumping range sets for a domain.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 xen/drivers/vpci/header.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 40ff79c33f8f..82a3e50d6053 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -206,12 +206,16 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
 static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 {
     struct vpci_header *header = &pdev->vpci->header;
-    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
+    struct rangeset *mem;
+    char str[32];
     struct pci_dev *tmp, *dev = NULL;
     const struct vpci_msix *msix = pdev->vpci->msix;
     unsigned int i;
     int rc;
 
+    snprintf(str, sizeof(str), "%pp", &pdev->sbdf);
+    mem = rangeset_new(NULL, str, RANGESETF_no_print);
+
     if ( !mem )
         return -ENOMEM;
 
-- 
2.25.1

Re: [PATCH 2/2] vpci: use named rangeset for BARs

[Roger Pau Monné]

On Mon, Nov 22, 2021 at 11:28:25AM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Use a named range set instead of an anonymous one, but do not print it
> while dumping range sets for a domain.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>  xen/drivers/vpci/header.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 40ff79c33f8f..82a3e50d6053 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -206,12 +206,16 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>  static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>  {
>      struct vpci_header *header = &pdev->vpci->header;
> -    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
> +    struct rangeset *mem;
> +    char str[32];
>      struct pci_dev *tmp, *dev = NULL;
>      const struct vpci_msix *msix = pdev->vpci->msix;
>      unsigned int i;
>      int rc;
>  
> +    snprintf(str, sizeof(str), "%pp", &pdev->sbdf);
> +    mem = rangeset_new(NULL, str, RANGESETF_no_print);

You are still not adding the rangeset to the domain list, as the first
parameter passed here in NULL instead of a domain struct.

Given the current short living of the rangesets I'm not sure it makes
much sense to link them to the domain ATM, but I guess this is kind of
a preparatory change as other patches you have will have the
rangesets permanent as long as the device is assigned to a domain.

Likely the above reasoning (or the appropriate one) should be added to
the commit message.

Thanks, Roger.

Re: [PATCH 2/2] vpci: use named rangeset for BARs

[Jan Beulich]

On 22.11.2021 11:27, Roger Pau Monné wrote:
> On Mon, Nov 22, 2021 at 11:28:25AM +0200, Oleksandr Andrushchenko wrote:
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -206,12 +206,16 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>>  static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>  {
>>      struct vpci_header *header = &pdev->vpci->header;
>> -    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>> +    struct rangeset *mem;
>> +    char str[32];
>>      struct pci_dev *tmp, *dev = NULL;
>>      const struct vpci_msix *msix = pdev->vpci->msix;
>>      unsigned int i;
>>      int rc;
>>  
>> +    snprintf(str, sizeof(str), "%pp", &pdev->sbdf);
>> +    mem = rangeset_new(NULL, str, RANGESETF_no_print);
> 
> You are still not adding the rangeset to the domain list, as the first
> parameter passed here in NULL instead of a domain struct.
> 
> Given the current short living of the rangesets I'm not sure it makes
> much sense to link them to the domain ATM, but I guess this is kind of
> a preparatory change as other patches you have will have the
> rangesets permanent as long as the device is assigned to a domain.
> 
> Likely the above reasoning (or the appropriate one) should be added to
> the commit message.

Or, as also suggested as an option, them getting accounted to the domain
could be folded into the patch making them long-lived.

Jan

Re: [PATCH 2/2] vpci: use named rangeset for BARs

[Oleksandr Andrushchenko]

On 22.11.21 12:43, Jan Beulich wrote:
> On 22.11.2021 11:27, Roger Pau Monné wrote:
>> On Mon, Nov 22, 2021 at 11:28:25AM +0200, Oleksandr Andrushchenko wrote:
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -206,12 +206,16 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>>>   static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>   {
>>>       struct vpci_header *header = &pdev->vpci->header;
>>> -    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>>> +    struct rangeset *mem;
>>> +    char str[32];
>>>       struct pci_dev *tmp, *dev = NULL;
>>>       const struct vpci_msix *msix = pdev->vpci->msix;
>>>       unsigned int i;
>>>       int rc;
>>>   
>>> +    snprintf(str, sizeof(str), "%pp", &pdev->sbdf);
>>> +    mem = rangeset_new(NULL, str, RANGESETF_no_print);
>> You are still not adding the rangeset to the domain list, as the first
>> parameter passed here in NULL instead of a domain struct.
>>
>> Given the current short living of the rangesets I'm not sure it makes
>> much sense to link them to the domain ATM, but I guess this is kind of
>> a preparatory change as other patches you have will have the
>> rangesets permanent as long as the device is assigned to a domain.
>>
>> Likely the above reasoning (or the appropriate one) should be added to
>> the commit message.
If I fold then there is no reason to add the comment, right?
> Or, as also suggested as an option, them getting accounted to the domain
> could be folded into the patch making them long-lived.
Ok, I can fold this patch and have:
@@ -95,10 +102,27 @@ int vpci_add_handlers(struct pci_dev *pdev)
      INIT_LIST_HEAD(&pdev->vpci->handlers);
      spin_lock_init(&pdev->vpci->lock);

+    header = &pdev->vpci->header;
+    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    {
+        struct vpci_bar *bar = &header->bars[i];
+        char str[32];
+
+        snprintf(str, sizeof(str), "%pp:BAR%d", &pdev->sbdf, i);
+        bar->mem = rangeset_new(pdev->domain, str, RANGESETF_no_print);

>
> Jan
>
Thank you,
Oleksandr

Re: [PATCH 2/2] vpci: use named rangeset for BARs

[Roger Pau Monné]

On Mon, Nov 22, 2021 at 10:50:18AM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> On 22.11.21 12:43, Jan Beulich wrote:
> > On 22.11.2021 11:27, Roger Pau Monné wrote:
> >> On Mon, Nov 22, 2021 at 11:28:25AM +0200, Oleksandr Andrushchenko wrote:
> >>> --- a/xen/drivers/vpci/header.c
> >>> +++ b/xen/drivers/vpci/header.c
> >>> @@ -206,12 +206,16 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
> >>>   static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
> >>>   {
> >>>       struct vpci_header *header = &pdev->vpci->header;
> >>> -    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
> >>> +    struct rangeset *mem;
> >>> +    char str[32];
> >>>       struct pci_dev *tmp, *dev = NULL;
> >>>       const struct vpci_msix *msix = pdev->vpci->msix;
> >>>       unsigned int i;
> >>>       int rc;
> >>>   
> >>> +    snprintf(str, sizeof(str), "%pp", &pdev->sbdf);
> >>> +    mem = rangeset_new(NULL, str, RANGESETF_no_print);
> >> You are still not adding the rangeset to the domain list, as the first
> >> parameter passed here in NULL instead of a domain struct.
> >>
> >> Given the current short living of the rangesets I'm not sure it makes
> >> much sense to link them to the domain ATM, but I guess this is kind of
> >> a preparatory change as other patches you have will have the
> >> rangesets permanent as long as the device is assigned to a domain.
> >>
> >> Likely the above reasoning (or the appropriate one) should be added to
> >> the commit message.
> If I fold then there is no reason to add the comment, right?

I find detailed log messages never hurt, so in the patch where you
squash the chunk below I would add that as part of making the
rangesets permanent they are also linked to the domain struct in order
to properly track them.

Thanks, Roger.

Re: [PATCH 2/2] vpci: use named rangeset for BARs

[Jan Beulich]

On 22.11.2021 11:50, Oleksandr Andrushchenko wrote:
> 
> 
> On 22.11.21 12:43, Jan Beulich wrote:
>> On 22.11.2021 11:27, Roger Pau Monné wrote:
>>> On Mon, Nov 22, 2021 at 11:28:25AM +0200, Oleksandr Andrushchenko wrote:
>>>> --- a/xen/drivers/vpci/header.c
>>>> +++ b/xen/drivers/vpci/header.c
>>>> @@ -206,12 +206,16 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>>>>   static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>>   {
>>>>       struct vpci_header *header = &pdev->vpci->header;
>>>> -    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>>>> +    struct rangeset *mem;
>>>> +    char str[32];
>>>>       struct pci_dev *tmp, *dev = NULL;
>>>>       const struct vpci_msix *msix = pdev->vpci->msix;
>>>>       unsigned int i;
>>>>       int rc;
>>>>   
>>>> +    snprintf(str, sizeof(str), "%pp", &pdev->sbdf);
>>>> +    mem = rangeset_new(NULL, str, RANGESETF_no_print);
>>> You are still not adding the rangeset to the domain list, as the first
>>> parameter passed here in NULL instead of a domain struct.
>>>
>>> Given the current short living of the rangesets I'm not sure it makes
>>> much sense to link them to the domain ATM, but I guess this is kind of
>>> a preparatory change as other patches you have will have the
>>> rangesets permanent as long as the device is assigned to a domain.
>>>
>>> Likely the above reasoning (or the appropriate one) should be added to
>>> the commit message.
> If I fold then there is no reason to add the comment, right?

I'd say you still want to justify the change from "anonymous" to "named and
accounted".

Jan

Re: [PATCH 2/2] vpci: use named rangeset for BARs

[Oleksandr Andrushchenko]

On 22.11.21 12:54, Jan Beulich wrote:
> On 22.11.2021 11:50, Oleksandr Andrushchenko wrote:
>>
>> On 22.11.21 12:43, Jan Beulich wrote:
>>> On 22.11.2021 11:27, Roger Pau Monné wrote:
>>>> On Mon, Nov 22, 2021 at 11:28:25AM +0200, Oleksandr Andrushchenko wrote:
>>>>> --- a/xen/drivers/vpci/header.c
>>>>> +++ b/xen/drivers/vpci/header.c
>>>>> @@ -206,12 +206,16 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>>>>>    static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>>>    {
>>>>>        struct vpci_header *header = &pdev->vpci->header;
>>>>> -    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>>>>> +    struct rangeset *mem;
>>>>> +    char str[32];
>>>>>        struct pci_dev *tmp, *dev = NULL;
>>>>>        const struct vpci_msix *msix = pdev->vpci->msix;
>>>>>        unsigned int i;
>>>>>        int rc;
>>>>>    
>>>>> +    snprintf(str, sizeof(str), "%pp", &pdev->sbdf);
>>>>> +    mem = rangeset_new(NULL, str, RANGESETF_no_print);
>>>> You are still not adding the rangeset to the domain list, as the first
>>>> parameter passed here in NULL instead of a domain struct.
>>>>
>>>> Given the current short living of the rangesets I'm not sure it makes
>>>> much sense to link them to the domain ATM, but I guess this is kind of
>>>> a preparatory change as other patches you have will have the
>>>> rangesets permanent as long as the device is assigned to a domain.
>>>>
>>>> Likely the above reasoning (or the appropriate one) should be added to
>>>> the commit message.
>> If I fold then there is no reason to add the comment, right?
> I'd say you still want to justify the change from "anonymous" to "named and
> accounted".
"Make the range sets permanent, e.g. create them when a PCI device is
added and destroy when it is removed. Also make the range sets named
and accounted."

Will this work in the commit message?
>
> Jan
>
>
Thank you,
Oleksandr

Re: [PATCH 2/2] vpci: use named rangeset for BARs

[Jan Beulich]

On 22.11.2021 11:59, Oleksandr Andrushchenko wrote:
> On 22.11.21 12:54, Jan Beulich wrote:
>> On 22.11.2021 11:50, Oleksandr Andrushchenko wrote:
>>>
>>> On 22.11.21 12:43, Jan Beulich wrote:
>>>> On 22.11.2021 11:27, Roger Pau Monné wrote:
>>>>> On Mon, Nov 22, 2021 at 11:28:25AM +0200, Oleksandr Andrushchenko wrote:
>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>> @@ -206,12 +206,16 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>>>>>>    static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>>>>    {
>>>>>>        struct vpci_header *header = &pdev->vpci->header;
>>>>>> -    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>>>>>> +    struct rangeset *mem;
>>>>>> +    char str[32];
>>>>>>        struct pci_dev *tmp, *dev = NULL;
>>>>>>        const struct vpci_msix *msix = pdev->vpci->msix;
>>>>>>        unsigned int i;
>>>>>>        int rc;
>>>>>>    
>>>>>> +    snprintf(str, sizeof(str), "%pp", &pdev->sbdf);
>>>>>> +    mem = rangeset_new(NULL, str, RANGESETF_no_print);
>>>>> You are still not adding the rangeset to the domain list, as the first
>>>>> parameter passed here in NULL instead of a domain struct.
>>>>>
>>>>> Given the current short living of the rangesets I'm not sure it makes
>>>>> much sense to link them to the domain ATM, but I guess this is kind of
>>>>> a preparatory change as other patches you have will have the
>>>>> rangesets permanent as long as the device is assigned to a domain.
>>>>>
>>>>> Likely the above reasoning (or the appropriate one) should be added to
>>>>> the commit message.
>>> If I fold then there is no reason to add the comment, right?
>> I'd say you still want to justify the change from "anonymous" to "named and
>> accounted".
> "Make the range sets permanent, e.g. create them when a PCI device is
> added and destroy when it is removed. Also make the range sets named
> and accounted."

Making them permanent is a requirement of your change iirc, so I'd start with
"Because the range sets now become permanent, make them ...".

Jan

Re: [PATCH 2/2] vpci: use named rangeset for BARs

[Oleksandr Andrushchenko]

On 22.11.21 13:08, Jan Beulich wrote:
> On 22.11.2021 11:59, Oleksandr Andrushchenko wrote:
>> On 22.11.21 12:54, Jan Beulich wrote:
>>> On 22.11.2021 11:50, Oleksandr Andrushchenko wrote:
>>>> On 22.11.21 12:43, Jan Beulich wrote:
>>>>> On 22.11.2021 11:27, Roger Pau Monné wrote:
>>>>>> On Mon, Nov 22, 2021 at 11:28:25AM +0200, Oleksandr Andrushchenko wrote:
>>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>>> @@ -206,12 +206,16 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>>>>>>>     static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>>>>>     {
>>>>>>>         struct vpci_header *header = &pdev->vpci->header;
>>>>>>> -    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>>>>>>> +    struct rangeset *mem;
>>>>>>> +    char str[32];
>>>>>>>         struct pci_dev *tmp, *dev = NULL;
>>>>>>>         const struct vpci_msix *msix = pdev->vpci->msix;
>>>>>>>         unsigned int i;
>>>>>>>         int rc;
>>>>>>>     
>>>>>>> +    snprintf(str, sizeof(str), "%pp", &pdev->sbdf);
>>>>>>> +    mem = rangeset_new(NULL, str, RANGESETF_no_print);
>>>>>> You are still not adding the rangeset to the domain list, as the first
>>>>>> parameter passed here in NULL instead of a domain struct.
>>>>>>
>>>>>> Given the current short living of the rangesets I'm not sure it makes
>>>>>> much sense to link them to the domain ATM, but I guess this is kind of
>>>>>> a preparatory change as other patches you have will have the
>>>>>> rangesets permanent as long as the device is assigned to a domain.
>>>>>>
>>>>>> Likely the above reasoning (or the appropriate one) should be added to
>>>>>> the commit message.
>>>> If I fold then there is no reason to add the comment, right?
>>> I'd say you still want to justify the change from "anonymous" to "named and
>>> accounted".
>> "Make the range sets permanent, e.g. create them when a PCI device is
>> added and destroy when it is removed. Also make the range sets named
>> and accounted."
> Making them permanent is a requirement of your change iirc, so I'd start with
> "Because the range sets now become permanent, make them ...".
"As the range sets are now created when a PCI device is added and destroyed
when it is removed so make them named and accounted."
>
> Jan
>
>
Thank you,
Oleksandr

Re: [PATCH 1/2] rangeset: add RANGESETF_no_print flag

[Oleksandr Andrushchenko]

On 22.11.21 11:28, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> There are range sets which should not be printed, so introduce a flag
> which allows marking those as such. Implement relevant logic to skip
> such entries while printing.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>   xen/common/rangeset.c      | 3 +++
>   xen/include/xen/rangeset.h | 3 +++
>   2 files changed, 6 insertions(+)
>
> diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
> index 885b6b15c229..939883a1d145 100644
> --- a/xen/common/rangeset.c
> +++ b/xen/common/rangeset.c
> @@ -575,6 +575,9 @@ void rangeset_domain_printk(
>   
>       list_for_each_entry ( r, &d->rangesets, rangeset_list )
>       {
> +        if ( r->flags & RANGESETF_no_print )
> +            continue;
> +
>           printk("    ");
>           rangeset_printk(r);
>           printk("\n");
> diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
> index 135f33f6066f..543540a88b6f 100644
> --- a/xen/include/xen/rangeset.h
> +++ b/xen/include/xen/rangeset.h
> @@ -51,6 +51,9 @@ void rangeset_limit(
>    /* Pretty-print range limits in hexadecimal. */
>   #define _RANGESETF_prettyprint_hex 0
>   #define RANGESETF_prettyprint_hex  (1U << _RANGESETF_prettyprint_hex)
> +/* Do not print entries marked with this flag. */
> +#define _RANGESETF_no_print 1
> +#define RANGESETF_no_print  (1U << _RANGESETF_no_print)
>   
>   bool_t __must_check rangeset_is_empty(
>       const struct rangeset *r);
This will also require:
diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index 939883a1d145..ea27d651723b 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -433,7 +433,7 @@ struct rangeset *rangeset_new(
      INIT_LIST_HEAD(&r->range_list);
      r->nr_ranges = -1;

-    BUG_ON(flags & ~RANGESETF_prettyprint_hex);
+    BUG_ON(flags & ~(RANGESETF_prettyprint_hex | RANGESETF_no_print));
      r->flags = flags;

Or we just remove BUG_ON now

Thank you,
Oleksandr

Re: [PATCH 1/2] rangeset: add RANGESETF_no_print flag

[Jan Beulich]

On 22.11.2021 10:28, Oleksandr Andrushchenko wrote:
> --- a/xen/include/xen/rangeset.h
> +++ b/xen/include/xen/rangeset.h
> @@ -51,6 +51,9 @@ void rangeset_limit(
>   /* Pretty-print range limits in hexadecimal. */
>  #define _RANGESETF_prettyprint_hex 0
>  #define RANGESETF_prettyprint_hex  (1U << _RANGESETF_prettyprint_hex)
> +/* Do not print entries marked with this flag. */
> +#define _RANGESETF_no_print 1

There's no need for this, so I'd like to ask that you add ...

> +#define RANGESETF_no_print  (1U << _RANGESETF_no_print)

... just this. In due course we should do away with
_RANGESETF_prettyprint_hex as well (unless you want to do so right
at this occasion).

Jan

Re: [PATCH 1/2] rangeset: add RANGESETF_no_print flag

[Oleksandr Andrushchenko]

On 23.11.21 09:38, Jan Beulich wrote:
> On 22.11.2021 10:28, Oleksandr Andrushchenko wrote:
>> --- a/xen/include/xen/rangeset.h
>> +++ b/xen/include/xen/rangeset.h
>> @@ -51,6 +51,9 @@ void rangeset_limit(
>>    /* Pretty-print range limits in hexadecimal. */
>>   #define _RANGESETF_prettyprint_hex 0
>>   #define RANGESETF_prettyprint_hex  (1U << _RANGESETF_prettyprint_hex)
>> +/* Do not print entries marked with this flag. */
>> +#define _RANGESETF_no_print 1
> There's no need for this, so I'd like to ask that you add ...
>
>> +#define RANGESETF_no_print  (1U << _RANGESETF_no_print)
> ... just this. In due course we should do away with
> _RANGESETF_prettyprint_hex as well (unless you want to do so right
> at this occasion).
Ok, I can remove "#define _RANGESETF_prettyprint_hex 0" as well
and have both flags defined as
  /* Pretty-print range limits in hexadecimal. */
#define RANGESETF_prettyprint_hex  (1U << 0)
/* Do not print entries marked with this flag. */
#define RANGESETF_no_print         (1U << 1)

Or we can use BIT macro here:

/* Pretty-print range limits in hexadecimal. */
#define RANGESETF_prettyprint_hex  BIT(0, U)
/* Do not print entries marked with this flag. */
#define RANGESETF_no_print         BIT(1, U)

What's your preference here?
>
> Jan
>
Thank you,
Oleksandr

Re: [PATCH 1/2] rangeset: add RANGESETF_no_print flag

[Jan Beulich]

On 23.11.2021 08:49, Oleksandr Andrushchenko wrote:
> 
> 
> On 23.11.21 09:38, Jan Beulich wrote:
>> On 22.11.2021 10:28, Oleksandr Andrushchenko wrote:
>>> --- a/xen/include/xen/rangeset.h
>>> +++ b/xen/include/xen/rangeset.h
>>> @@ -51,6 +51,9 @@ void rangeset_limit(
>>>    /* Pretty-print range limits in hexadecimal. */
>>>   #define _RANGESETF_prettyprint_hex 0
>>>   #define RANGESETF_prettyprint_hex  (1U << _RANGESETF_prettyprint_hex)
>>> +/* Do not print entries marked with this flag. */
>>> +#define _RANGESETF_no_print 1
>> There's no need for this, so I'd like to ask that you add ...
>>
>>> +#define RANGESETF_no_print  (1U << _RANGESETF_no_print)
>> ... just this. In due course we should do away with
>> _RANGESETF_prettyprint_hex as well (unless you want to do so right
>> at this occasion).
> Ok, I can remove "#define _RANGESETF_prettyprint_hex 0" as well
> and have both flags defined as
>   /* Pretty-print range limits in hexadecimal. */
> #define RANGESETF_prettyprint_hex  (1U << 0)
> /* Do not print entries marked with this flag. */
> #define RANGESETF_no_print         (1U << 1)
> 
> Or we can use BIT macro here:
> 
> /* Pretty-print range limits in hexadecimal. */
> #define RANGESETF_prettyprint_hex  BIT(0, U)
> /* Do not print entries marked with this flag. */
> #define RANGESETF_no_print         BIT(1, U)
> 
> What's your preference here?

Clearly the former. But this may not be everybody's view. I'm hesitant
towards further uses at least as long as its generalization, GENMASK(),
isn't ready for easy use. It was some time ago that we had a discussion
about that one, supporting my reservations voiced back at the time when
it was introduced into our code base. Iirc there was more than one
issue with it; the immediately obvious one is that it silently does the
wrong thing when the arguments get specified the wrong way round, when
it really would be relatively easy to support either ordering.

Jan

Re: [PATCH 1/2] rangeset: add RANGESETF_no_print flag

[Oleksandr Andrushchenko]

On 23.11.21 10:01, Jan Beulich wrote:
> On 23.11.2021 08:49, Oleksandr Andrushchenko wrote:
>>
>> On 23.11.21 09:38, Jan Beulich wrote:
>>> On 22.11.2021 10:28, Oleksandr Andrushchenko wrote:
>>>> --- a/xen/include/xen/rangeset.h
>>>> +++ b/xen/include/xen/rangeset.h
>>>> @@ -51,6 +51,9 @@ void rangeset_limit(
>>>>     /* Pretty-print range limits in hexadecimal. */
>>>>    #define _RANGESETF_prettyprint_hex 0
>>>>    #define RANGESETF_prettyprint_hex  (1U << _RANGESETF_prettyprint_hex)
>>>> +/* Do not print entries marked with this flag. */
>>>> +#define _RANGESETF_no_print 1
>>> There's no need for this, so I'd like to ask that you add ...
>>>
>>>> +#define RANGESETF_no_print  (1U << _RANGESETF_no_print)
>>> ... just this. In due course we should do away with
>>> _RANGESETF_prettyprint_hex as well (unless you want to do so right
>>> at this occasion).
>> Ok, I can remove "#define _RANGESETF_prettyprint_hex 0" as well
>> and have both flags defined as
>>    /* Pretty-print range limits in hexadecimal. */
>> #define RANGESETF_prettyprint_hex  (1U << 0)
>> /* Do not print entries marked with this flag. */
>> #define RANGESETF_no_print         (1U << 1)
>>
>> Or we can use BIT macro here:
>>
>> /* Pretty-print range limits in hexadecimal. */
>> #define RANGESETF_prettyprint_hex  BIT(0, U)
>> /* Do not print entries marked with this flag. */
>> #define RANGESETF_no_print         BIT(1, U)
>>
>> What's your preference here?
> Clearly the former.
I am fine with this too, so I'll use it
>   But this may not be everybody's view. I'm hesitant
> towards further uses at least as long as its generalization, GENMASK(),
> isn't ready for easy use. It was some time ago that we had a discussion
> about that one, supporting my reservations voiced back at the time when
> it was introduced into our code base. Iirc there was more than one
> issue with it; the immediately obvious one is that it silently does the
> wrong thing when the arguments get specified the wrong way round, when
> it really would be relatively easy to support either ordering.
>
> Jan
>
Thank you,
Oleksandr