| Message ID | 20211122092825.2502306-2-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] rangeset: add RANGESETF_no_print flag | expand |
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.
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
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
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.
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
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
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
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
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;