Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
"Michael S. Tsirkin" writes: >> virtio: don't crash when device is buggy >> >> Because of a sanity check in virtio_dev_remove, a buggy device can crash >> kernel. And in case of rproc it's userspace so it's not a good idea. >> We are unloading a driver so how bad can it be? >> Be less aggressive in handling this error: if it's a driver bug, >> warning once should be enough. >> >> Signed-off-by: Michael S. Tsirkin > > Rusty? Thanks, applied. I really want to implement CONFIG_VIRTIO_DEVEL_DEBUG which would incorporate the checks in virtio_ring.c as well as this. Then I could also reshuffle descriptors (eg. split them) to find buggy devices like qemu which assume the first descriptor is the struct virtio_net_hdr... Cheers, Rusty, -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Michael S. Tsirkin m...@redhat.com writes: virtio: don't crash when device is buggy Because of a sanity check in virtio_dev_remove, a buggy device can crash kernel. And in case of rproc it's userspace so it's not a good idea. We are unloading a driver so how bad can it be? Be less aggressive in handling this error: if it's a driver bug, warning once should be enough. Signed-off-by: Michael S. Tsirkin m...@redhat.com Rusty? Thanks, applied. I really want to implement CONFIG_VIRTIO_DEVEL_DEBUG which would incorporate the checks in virtio_ring.c as well as this. Then I could also reshuffle descriptors (eg. split them) to find buggy devices like qemu which assume the first descriptor is the struct virtio_net_hdr... Cheers, Rusty, -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Thu, Sep 06, 2012 at 08:27:35AM +0300, Michael S. Tsirkin wrote: > On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote: > > "Michael S. Tsirkin" writes: > > > On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: > > >> Hi Michael, > > >> > > >> > Exactly. Though if we just fail load it will be much less code. > > >> > > > >> > Generally, using a feature bit for this is a bit of a problem though: > > >> > normally driver is expected to be able to simply ignore > > >> > a feature bit. In this case driver is required to > > >> > do something so a feature bit is not a good fit. > > >> > I am not sure what the right thing to do is. > > >> > > >> I see - so in order to avoid the binding between driver and device > > >> there are two options I guess. Either make virtio_dev_match() or > > >> virtcons_probe() fail. Neither of them seems like the obvious choice. > > >> > > >> Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match > > >> between device and driver in virtcons_probe() is the lesser evil? > > >> > > >> Regards, > > >> Sjur > > > > > > A simplest thing to do is change dev id. rusty? > > > > For generic usage, this is correct. But my opinion is that fallback on > > feature non-ack is quality-of-implementation issue: great to have, but > > there are cases where you just want to fail with "you're too old". > > > > And in this case, an old system simply will never work. So it's a > > question of how graceful the failure is. > > > > Can your userspace loader can refuse to proceed if the driver doesn't > > ack the bits? If so, it's simpler than a whole new ID. > > > > Cheers, > > Rusty. > > Yes but how can it signal guest that it will never proceed? > > Also grep for BUG_ON in core found this: > >drv->remove(dev); > >/* Driver should have reset device. */ >BUG_ON(dev->config->get_status(dev)); > > I think below is what Sjur refers to. > I think below is a good idea for 3.6. Thoughts? > > ---> > > virtio: don't crash when device is buggy > > Because of a sanity check in virtio_dev_remove, a buggy device can crash > kernel. And in case of rproc it's userspace so it's not a good idea. > We are unloading a driver so how bad can it be? > Be less aggressive in handling this error: if it's a driver bug, > warning once should be enough. > > Signed-off-by: Michael S. Tsirkin > > -- Rusty? > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index c3b3f7f..1e8659c 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -159,7 +159,7 @@ static int virtio_dev_remove(struct device *_d) > drv->remove(dev); > > /* Driver should have reset device. */ > - BUG_ON(dev->config->get_status(dev)); > + WARN_ON_ONCE(dev->config->get_status(dev)); > > /* Acknowledge the device's existence again. */ > add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); > > -- > MST -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Thu, Sep 06, 2012 at 08:27:35AM +0300, Michael S. Tsirkin wrote: On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote: Michael S. Tsirkin m...@redhat.com writes: On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: Hi Michael, Exactly. Though if we just fail load it will be much less code. Generally, using a feature bit for this is a bit of a problem though: normally driver is expected to be able to simply ignore a feature bit. In this case driver is required to do something so a feature bit is not a good fit. I am not sure what the right thing to do is. I see - so in order to avoid the binding between driver and device there are two options I guess. Either make virtio_dev_match() or virtcons_probe() fail. Neither of them seems like the obvious choice. Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match between device and driver in virtcons_probe() is the lesser evil? Regards, Sjur A simplest thing to do is change dev id. rusty? For generic usage, this is correct. But my opinion is that fallback on feature non-ack is quality-of-implementation issue: great to have, but there are cases where you just want to fail with you're too old. And in this case, an old system simply will never work. So it's a question of how graceful the failure is. Can your userspace loader can refuse to proceed if the driver doesn't ack the bits? If so, it's simpler than a whole new ID. Cheers, Rusty. Yes but how can it signal guest that it will never proceed? Also grep for BUG_ON in core found this: drv-remove(dev); /* Driver should have reset device. */ BUG_ON(dev-config-get_status(dev)); I think below is what Sjur refers to. I think below is a good idea for 3.6. Thoughts? --- virtio: don't crash when device is buggy Because of a sanity check in virtio_dev_remove, a buggy device can crash kernel. And in case of rproc it's userspace so it's not a good idea. We are unloading a driver so how bad can it be? Be less aggressive in handling this error: if it's a driver bug, warning once should be enough. Signed-off-by: Michael S. Tsirkin m...@redhat.com -- Rusty? diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index c3b3f7f..1e8659c 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -159,7 +159,7 @@ static int virtio_dev_remove(struct device *_d) drv-remove(dev); /* Driver should have reset device. */ - BUG_ON(dev-config-get_status(dev)); + WARN_ON_ONCE(dev-config-get_status(dev)); /* Acknowledge the device's existence again. */ add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); -- MST -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael.
>> Not just fail to work, the kernel will panic on the BUG_ON().
>> Remoteproc gets the virtio configuration from firmware loaded
>> from user space. So this type of problem might be triggered
>> for other virtio drivers as well.
>
> how?
...
>> Even if we fix this particular problem, the general problem
>> still exists: bogus virtio declarations in remoteproc's firmware
>> may cause BUG_ON().
>
> which BUG_ON exactly?
I am afraid I have been barking up the wrong tree here.
Please ignore my previous rambling about panics related
to device features.
I hit the BUG() in virtio_check_driver_offered_feature():
First I did not declare the feature because DMA was not set:
static unsigned int features[] = {
...
#if VIRTIO_CONSOLE_HAS_DMA
VIRTIO_CONSOLE_F_DMA_MEM,
#endif
};
and then in probe I checked if the feature was supported:
virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)
This triggered the BUG() in virtio_check_driver_offered_feature(),
because the driver was asking for a unknown-feature.
I can get avoid this by simply checking the devices feature bits directly
instead of using virtio_has_feature().
Regards,
Sjur
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Ohad, >> A simplest thing to do is change dev id. rusty? > > For generic usage, this is correct. But my opinion is that fallback on > feature non-ack is quality-of-implementation issue: great to have, but > there are cases where you just want to fail with "you're too old". > > And in this case, an old system simply will never work. So it's a > question of how graceful the failure is. > > Can your userspace loader can refuse to proceed if the driver doesn't > ack the bits? If so, it's simpler than a whole new ID. Ohad: Are there any way we could avoid starting up the remote processor if mandatory virtio features (such as DMA memory) are not supported by the virtio-driver? Regards, Sjur -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Ohad, A simplest thing to do is change dev id. rusty? For generic usage, this is correct. But my opinion is that fallback on feature non-ack is quality-of-implementation issue: great to have, but there are cases where you just want to fail with you're too old. And in this case, an old system simply will never work. So it's a question of how graceful the failure is. Can your userspace loader can refuse to proceed if the driver doesn't ack the bits? If so, it's simpler than a whole new ID. Ohad: Are there any way we could avoid starting up the remote processor if mandatory virtio features (such as DMA memory) are not supported by the virtio-driver? Regards, Sjur -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael.
Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.
how?
...
Even if we fix this particular problem, the general problem
still exists: bogus virtio declarations in remoteproc's firmware
may cause BUG_ON().
which BUG_ON exactly?
I am afraid I have been barking up the wrong tree here.
Please ignore my previous rambling about panics related
to device features.
I hit the BUG() in virtio_check_driver_offered_feature():
First I did not declare the feature because DMA was not set:
static unsigned int features[] = {
...
#if VIRTIO_CONSOLE_HAS_DMA
VIRTIO_CONSOLE_F_DMA_MEM,
#endif
};
and then in probe I checked if the feature was supported:
virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)
This triggered the BUG() in virtio_check_driver_offered_feature(),
because the driver was asking for a unknown-feature.
I can get avoid this by simply checking the devices feature bits directly
instead of using virtio_has_feature().
Regards,
Sjur
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote: > "Michael S. Tsirkin" writes: > > On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: > >> Hi Michael, > >> > >> > Exactly. Though if we just fail load it will be much less code. > >> > > >> > Generally, using a feature bit for this is a bit of a problem though: > >> > normally driver is expected to be able to simply ignore > >> > a feature bit. In this case driver is required to > >> > do something so a feature bit is not a good fit. > >> > I am not sure what the right thing to do is. > >> > >> I see - so in order to avoid the binding between driver and device > >> there are two options I guess. Either make virtio_dev_match() or > >> virtcons_probe() fail. Neither of them seems like the obvious choice. > >> > >> Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match > >> between device and driver in virtcons_probe() is the lesser evil? > >> > >> Regards, > >> Sjur > > > > A simplest thing to do is change dev id. rusty? > > For generic usage, this is correct. But my opinion is that fallback on > feature non-ack is quality-of-implementation issue: great to have, but > there are cases where you just want to fail with "you're too old". > > And in this case, an old system simply will never work. So it's a > question of how graceful the failure is. > > Can your userspace loader can refuse to proceed if the driver doesn't > ack the bits? If so, it's simpler than a whole new ID. > > Cheers, > Rusty. Yes but how can it signal guest that it will never proceed? Also grep for BUG_ON in core found this: drv->remove(dev); /* Driver should have reset device. */ BUG_ON(dev->config->get_status(dev)); I think below is what Sjur refers to. I think below is a good idea for 3.6. Thoughts? ---> virtio: don't crash when device is buggy Because of a sanity check in virtio_dev_remove, a buggy device can crash kernel. And in case of rproc it's userspace so it's not a good idea. We are unloading a driver so how bad can it be? Be less aggressive in handling this error: if it's a driver bug, warning once should be enough. Signed-off-by: Michael S. Tsirkin -- diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index c3b3f7f..1e8659c 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -159,7 +159,7 @@ static int virtio_dev_remove(struct device *_d) drv->remove(dev); /* Driver should have reset device. */ - BUG_ON(dev->config->get_status(dev)); + WARN_ON_ONCE(dev->config->get_status(dev)); /* Acknowledge the device's existence again. */ add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); -- MST -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
"Michael S. Tsirkin" writes: > On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: >> Hi Michael, >> >> > Exactly. Though if we just fail load it will be much less code. >> > >> > Generally, using a feature bit for this is a bit of a problem though: >> > normally driver is expected to be able to simply ignore >> > a feature bit. In this case driver is required to >> > do something so a feature bit is not a good fit. >> > I am not sure what the right thing to do is. >> >> I see - so in order to avoid the binding between driver and device >> there are two options I guess. Either make virtio_dev_match() or >> virtcons_probe() fail. Neither of them seems like the obvious choice. >> >> Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match >> between device and driver in virtcons_probe() is the lesser evil? >> >> Regards, >> Sjur > > A simplest thing to do is change dev id. rusty? For generic usage, this is correct. But my opinion is that fallback on feature non-ack is quality-of-implementation issue: great to have, but there are cases where you just want to fail with "you're too old". And in this case, an old system simply will never work. So it's a question of how graceful the failure is. Can your userspace loader can refuse to proceed if the driver doesn't ack the bits? If so, it's simpler than a whole new ID. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Wed, Sep 05, 2012 at 08:15:36PM +0200, Sjur Brændeland wrote:
> Hi Michael.
>
> >> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
> >> when DMA is not supported, virtio will do BUG_ON() from
> >> virtio_check_driver_offered_feature().
> >>
> >> Is this acceptable or should we add a check in virtcons_probe()
> >> and let the probing fail instead?
> >>
> >> E.g:
> >> /* Refuse to bind if F_DMA_MEM request cannot be met */
> >> if (!VIRTIO_CONSOLE_HAS_DMA &&
> >> (vdev->config->get_features(vdev) & (1 <<
> >> VIRTIO_CONSOLE_F_DMA_MEM))){
> >> dev_err(>dev,
> >> "DMA_MEM requested, but arch does not support
> >> DMA\n");
> >> err = -EINVAL;
> >> goto fail;
> >> }
> >>
> >> Regards,
> >> Sjur
> >
> > Failing probe would be cleaner. But there is still a problem:
> > old driver will happily bind to that device and then
> > fail to work, right?
>
> Not just fail to work, the kernel will panic on the BUG_ON().
> Remoteproc gets the virtio configuration from firmware loaded
> from user space. So this type of problem might be triggered
> for other virtio drivers as well.
how?
>
> > virtio pci has revision id for this, but remoteproc doesn't
> > seem to have anything similar. Or did I miss it?
>
> No there are currently no sanity check of
> virtio type and feature bits in remoteproc.
> One option may be to add this...
you can not fix the past.
> > If not -
> > we probably need to use a different
> > device id, and not a feature bit.
>
> But if I create a new virtio console type, remoteproc
> could still call the existing virtio_console with random
> bad feature bits, causing kernel panic.
cirtio core checks device id - this should not happen.
> Even if we fix this particular problem, the general problem
> still exists: bogus virtio declarations in remoteproc's firmware
> may cause BUG_ON().
which BUG_ON exactly?
> (Note the fundamental difference
> between visualizations and remoteproc. For remoteproc
> the virtio configuration comes from binaries loaded from
> user space).
>
> So maybe we should look for a more generic solution, e.g.
> changing virtio probe functionality so that devices with
> bad feature bits will not trigger BUG_ON(), but rather refuse
> to bind the driver.
>
> Regards,
> Sjur
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael.
>> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
>> when DMA is not supported, virtio will do BUG_ON() from
>> virtio_check_driver_offered_feature().
>>
>> Is this acceptable or should we add a check in virtcons_probe()
>> and let the probing fail instead?
>>
>> E.g:
>> /* Refuse to bind if F_DMA_MEM request cannot be met */
>> if (!VIRTIO_CONSOLE_HAS_DMA &&
>> (vdev->config->get_features(vdev) & (1 <<
>> VIRTIO_CONSOLE_F_DMA_MEM))){
>> dev_err(>dev,
>> "DMA_MEM requested, but arch does not support DMA\n");
>> err = -EINVAL;
>> goto fail;
>> }
>>
>> Regards,
>> Sjur
>
> Failing probe would be cleaner. But there is still a problem:
> old driver will happily bind to that device and then
> fail to work, right?
Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.
> virtio pci has revision id for this, but remoteproc doesn't
> seem to have anything similar. Or did I miss it?
No there are currently no sanity check of
virtio type and feature bits in remoteproc.
One option may be to add this...
> If not -
> we probably need to use a different
> device id, and not a feature bit.
But if I create a new virtio console type, remoteproc
could still call the existing virtio_console with random
bad feature bits, causing kernel panic.
Even if we fix this particular problem, the general problem
still exists: bogus virtio declarations in remoteproc's firmware
may cause BUG_ON(). (Note the fundamental difference
between visualizations and remoteproc. For remoteproc
the virtio configuration comes from binaries loaded from
user space).
So maybe we should look for a more generic solution, e.g.
changing virtio probe functionality so that devices with
bad feature bits will not trigger BUG_ON(), but rather refuse
to bind the driver.
Regards,
Sjur
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Wed, Sep 05, 2012 at 03:00:20PM +0200, Sjur Brændeland wrote:
> > The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
> > don't have DMA!
>
> OK, so the feature table could be done like this:
>
> static unsigned int features[] = {
> VIRTIO_CONSOLE_F_SIZE,
> VIRTIO_CONSOLE_F_MULTIPORT,
> #if VIRTIO_CONSOLE_HAS_DMA
> VIRTIO_CONSOLE_F_DMA_MEM,
> #endif
> }
>
> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
> when DMA is not supported, virtio will do BUG_ON() from
> virtio_check_driver_offered_feature().
>
> Is this acceptable or should we add a check in virtcons_probe()
> and let the probing fail instead?
>
> E.g:
> /* Refuse to bind if F_DMA_MEM request cannot be met */
> if (!VIRTIO_CONSOLE_HAS_DMA &&
> (vdev->config->get_features(vdev) & (1 <<
> VIRTIO_CONSOLE_F_DMA_MEM))){
> dev_err(>dev,
> "DMA_MEM requested, but arch does not support DMA\n");
> err = -EINVAL;
> goto fail;
> }
>
> Regards,
> Sjur
Failing probe would be cleaner. But there is still a problem:
old driver will happily bind to that device and then
fail to work, right?
virtio pci has revision id for this, but remoteproc doesn't
seem to have anything similar. Or did I miss it? If not -
we probably need to use a different
device id, and not a feature bit.
--
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
> The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
> don't have DMA!
OK, so the feature table could be done like this:
static unsigned int features[] = {
VIRTIO_CONSOLE_F_SIZE,
VIRTIO_CONSOLE_F_MULTIPORT,
#if VIRTIO_CONSOLE_HAS_DMA
VIRTIO_CONSOLE_F_DMA_MEM,
#endif
}
If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
when DMA is not supported, virtio will do BUG_ON() from
virtio_check_driver_offered_feature().
Is this acceptable or should we add a check in virtcons_probe()
and let the probing fail instead?
E.g:
/* Refuse to bind if F_DMA_MEM request cannot be met */
if (!VIRTIO_CONSOLE_HAS_DMA &&
(vdev->config->get_features(vdev) & (1 <<
VIRTIO_CONSOLE_F_DMA_MEM))){
dev_err(>dev,
"DMA_MEM requested, but arch does not support DMA\n");
err = -EINVAL;
goto fail;
}
Regards,
Sjur
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Michael S. Tsirkin m...@redhat.com writes: On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: Hi Michael, Exactly. Though if we just fail load it will be much less code. Generally, using a feature bit for this is a bit of a problem though: normally driver is expected to be able to simply ignore a feature bit. In this case driver is required to do something so a feature bit is not a good fit. I am not sure what the right thing to do is. I see - so in order to avoid the binding between driver and device there are two options I guess. Either make virtio_dev_match() or virtcons_probe() fail. Neither of them seems like the obvious choice. Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match between device and driver in virtcons_probe() is the lesser evil? Regards, Sjur A simplest thing to do is change dev id. rusty? For generic usage, this is correct. But my opinion is that fallback on feature non-ack is quality-of-implementation issue: great to have, but there are cases where you just want to fail with you're too old. And in this case, an old system simply will never work. So it's a question of how graceful the failure is. Can your userspace loader can refuse to proceed if the driver doesn't ack the bits? If so, it's simpler than a whole new ID. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote: Michael S. Tsirkin m...@redhat.com writes: On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: Hi Michael, Exactly. Though if we just fail load it will be much less code. Generally, using a feature bit for this is a bit of a problem though: normally driver is expected to be able to simply ignore a feature bit. In this case driver is required to do something so a feature bit is not a good fit. I am not sure what the right thing to do is. I see - so in order to avoid the binding between driver and device there are two options I guess. Either make virtio_dev_match() or virtcons_probe() fail. Neither of them seems like the obvious choice. Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match between device and driver in virtcons_probe() is the lesser evil? Regards, Sjur A simplest thing to do is change dev id. rusty? For generic usage, this is correct. But my opinion is that fallback on feature non-ack is quality-of-implementation issue: great to have, but there are cases where you just want to fail with you're too old. And in this case, an old system simply will never work. So it's a question of how graceful the failure is. Can your userspace loader can refuse to proceed if the driver doesn't ack the bits? If so, it's simpler than a whole new ID. Cheers, Rusty. Yes but how can it signal guest that it will never proceed? Also grep for BUG_ON in core found this: drv-remove(dev); /* Driver should have reset device. */ BUG_ON(dev-config-get_status(dev)); I think below is what Sjur refers to. I think below is a good idea for 3.6. Thoughts? --- virtio: don't crash when device is buggy Because of a sanity check in virtio_dev_remove, a buggy device can crash kernel. And in case of rproc it's userspace so it's not a good idea. We are unloading a driver so how bad can it be? Be less aggressive in handling this error: if it's a driver bug, warning once should be enough. Signed-off-by: Michael S. Tsirkin m...@redhat.com -- diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index c3b3f7f..1e8659c 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -159,7 +159,7 @@ static int virtio_dev_remove(struct device *_d) drv-remove(dev); /* Driver should have reset device. */ - BUG_ON(dev-config-get_status(dev)); + WARN_ON_ONCE(dev-config-get_status(dev)); /* Acknowledge the device's existence again. */ add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); -- MST -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
don't have DMA!
OK, so the feature table could be done like this:
static unsigned int features[] = {
VIRTIO_CONSOLE_F_SIZE,
VIRTIO_CONSOLE_F_MULTIPORT,
#if VIRTIO_CONSOLE_HAS_DMA
VIRTIO_CONSOLE_F_DMA_MEM,
#endif
}
If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
when DMA is not supported, virtio will do BUG_ON() from
virtio_check_driver_offered_feature().
Is this acceptable or should we add a check in virtcons_probe()
and let the probing fail instead?
E.g:
/* Refuse to bind if F_DMA_MEM request cannot be met */
if (!VIRTIO_CONSOLE_HAS_DMA
(vdev-config-get_features(vdev) (1
VIRTIO_CONSOLE_F_DMA_MEM))){
dev_err(vdev-dev,
DMA_MEM requested, but arch does not support DMA\n);
err = -EINVAL;
goto fail;
}
Regards,
Sjur
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Wed, Sep 05, 2012 at 03:00:20PM +0200, Sjur Brændeland wrote:
The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
don't have DMA!
OK, so the feature table could be done like this:
static unsigned int features[] = {
VIRTIO_CONSOLE_F_SIZE,
VIRTIO_CONSOLE_F_MULTIPORT,
#if VIRTIO_CONSOLE_HAS_DMA
VIRTIO_CONSOLE_F_DMA_MEM,
#endif
}
If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
when DMA is not supported, virtio will do BUG_ON() from
virtio_check_driver_offered_feature().
Is this acceptable or should we add a check in virtcons_probe()
and let the probing fail instead?
E.g:
/* Refuse to bind if F_DMA_MEM request cannot be met */
if (!VIRTIO_CONSOLE_HAS_DMA
(vdev-config-get_features(vdev) (1
VIRTIO_CONSOLE_F_DMA_MEM))){
dev_err(vdev-dev,
DMA_MEM requested, but arch does not support DMA\n);
err = -EINVAL;
goto fail;
}
Regards,
Sjur
Failing probe would be cleaner. But there is still a problem:
old driver will happily bind to that device and then
fail to work, right?
virtio pci has revision id for this, but remoteproc doesn't
seem to have anything similar. Or did I miss it? If not -
we probably need to use a different
device id, and not a feature bit.
--
MST
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael.
If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
when DMA is not supported, virtio will do BUG_ON() from
virtio_check_driver_offered_feature().
Is this acceptable or should we add a check in virtcons_probe()
and let the probing fail instead?
E.g:
/* Refuse to bind if F_DMA_MEM request cannot be met */
if (!VIRTIO_CONSOLE_HAS_DMA
(vdev-config-get_features(vdev) (1
VIRTIO_CONSOLE_F_DMA_MEM))){
dev_err(vdev-dev,
DMA_MEM requested, but arch does not support DMA\n);
err = -EINVAL;
goto fail;
}
Regards,
Sjur
Failing probe would be cleaner. But there is still a problem:
old driver will happily bind to that device and then
fail to work, right?
Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.
virtio pci has revision id for this, but remoteproc doesn't
seem to have anything similar. Or did I miss it?
No there are currently no sanity check of
virtio type and feature bits in remoteproc.
One option may be to add this...
If not -
we probably need to use a different
device id, and not a feature bit.
But if I create a new virtio console type, remoteproc
could still call the existing virtio_console with random
bad feature bits, causing kernel panic.
Even if we fix this particular problem, the general problem
still exists: bogus virtio declarations in remoteproc's firmware
may cause BUG_ON(). (Note the fundamental difference
between visualizations and remoteproc. For remoteproc
the virtio configuration comes from binaries loaded from
user space).
So maybe we should look for a more generic solution, e.g.
changing virtio probe functionality so that devices with
bad feature bits will not trigger BUG_ON(), but rather refuse
to bind the driver.
Regards,
Sjur
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Wed, Sep 05, 2012 at 08:15:36PM +0200, Sjur Brændeland wrote:
Hi Michael.
If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
when DMA is not supported, virtio will do BUG_ON() from
virtio_check_driver_offered_feature().
Is this acceptable or should we add a check in virtcons_probe()
and let the probing fail instead?
E.g:
/* Refuse to bind if F_DMA_MEM request cannot be met */
if (!VIRTIO_CONSOLE_HAS_DMA
(vdev-config-get_features(vdev) (1
VIRTIO_CONSOLE_F_DMA_MEM))){
dev_err(vdev-dev,
DMA_MEM requested, but arch does not support
DMA\n);
err = -EINVAL;
goto fail;
}
Regards,
Sjur
Failing probe would be cleaner. But there is still a problem:
old driver will happily bind to that device and then
fail to work, right?
Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.
how?
virtio pci has revision id for this, but remoteproc doesn't
seem to have anything similar. Or did I miss it?
No there are currently no sanity check of
virtio type and feature bits in remoteproc.
One option may be to add this...
you can not fix the past.
If not -
we probably need to use a different
device id, and not a feature bit.
But if I create a new virtio console type, remoteproc
could still call the existing virtio_console with random
bad feature bits, causing kernel panic.
cirtio core checks device id - this should not happen.
Even if we fix this particular problem, the general problem
still exists: bogus virtio declarations in remoteproc's firmware
may cause BUG_ON().
which BUG_ON exactly?
(Note the fundamental difference
between visualizations and remoteproc. For remoteproc
the virtio configuration comes from binaries loaded from
user space).
So maybe we should look for a more generic solution, e.g.
changing virtio probe functionality so that devices with
bad feature bits will not trigger BUG_ON(), but rather refuse
to bind the driver.
Regards,
Sjur
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
"Michael S. Tsirkin" writes: > Also let's add a wrapper at top of file so ifdefs > do not litter the code like this. For example: > > #ifdef CONFIG_HAS_DMA > #define VIRTIO_CONSOLE_HAS_DMA (1) > #else > #define VIRTIO_CONSOLE_HAS_DMA (0) > #endif > > Now use if instead of ifdef. The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you don't have DMA! Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: > Hi Michael, > > > Exactly. Though if we just fail load it will be much less code. > > > > Generally, using a feature bit for this is a bit of a problem though: > > normally driver is expected to be able to simply ignore > > a feature bit. In this case driver is required to > > do something so a feature bit is not a good fit. > > I am not sure what the right thing to do is. > > I see - so in order to avoid the binding between driver and device > there are two options I guess. Either make virtio_dev_match() or > virtcons_probe() fail. Neither of them seems like the obvious choice. > > Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match > between device and driver in virtcons_probe() is the lesser evil? > > Regards, > Sjur A simplest thing to do is change dev id. rusty? -- MST -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael, > Exactly. Though if we just fail load it will be much less code. > > Generally, using a feature bit for this is a bit of a problem though: > normally driver is expected to be able to simply ignore > a feature bit. In this case driver is required to > do something so a feature bit is not a good fit. > I am not sure what the right thing to do is. I see - so in order to avoid the binding between driver and device there are two options I guess. Either make virtio_dev_match() or virtcons_probe() fail. Neither of them seems like the obvious choice. Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match between device and driver in virtcons_probe() is the lesser evil? Regards, Sjur -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Tue, Sep 04, 2012 at 01:28:36PM +0200, Sjur Brændeland wrote:
> Hi Michael,
>
> >> If an architecture do not support DMA you will get
> >> a link error: "unknown symbol" for dma_alloc_coherent.
> >
> > Yes, it even seems intentional.
> > But I have a question: can the device work without DMA?
>
> The main dependency is actually on the dma-allocation.
> In my case I do dma_declare_coherent_memory() with
> the memory area shared with the remote device as argument.
> Subsequent calls to dma_alloc_coherent() will then allocate
> from this memory area.
>
> > Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
>
> Yes.dma_alloc_coherent to work.
>
> > If yes you should just fail load.
>
> Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding
> VIRTIO_CONSOLE_F_DMA_MEM to the feature array.
>
> > Also let's add a wrapper at top of file so ifdefs
> > do not litter the code like this. For example:
> >
> > #ifdef CONFIG_HAS_DMA
> > #define VIRTIO_CONSOLE_HAS_DMA (1)
> > #else
> > #define VIRTIO_CONSOLE_HAS_DMA (0)
> > #endif
>
> OK, good point. Perhaps we could even exploit gcc's
> ability to remove dead code and do something like this:
>
>if (VIRTIO_CONSOLE_HAS_DMA &&
>virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
> ...
>
> This does not give any linker warnings when compiling for UML (no DMA).
>
> Regards,
> Sjur
Exactly. Though if we just fail load it will be much less code.
Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.
--
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael,
>> If an architecture do not support DMA you will get
>> a link error: "unknown symbol" for dma_alloc_coherent.
>
> Yes, it even seems intentional.
> But I have a question: can the device work without DMA?
The main dependency is actually on the dma-allocation.
In my case I do dma_declare_coherent_memory() with
the memory area shared with the remote device as argument.
Subsequent calls to dma_alloc_coherent() will then allocate
from this memory area.
> Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
Yes.dma_alloc_coherent to work.
> If yes you should just fail load.
Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding
VIRTIO_CONSOLE_F_DMA_MEM to the feature array.
> Also let's add a wrapper at top of file so ifdefs
> do not litter the code like this. For example:
>
> #ifdef CONFIG_HAS_DMA
> #define VIRTIO_CONSOLE_HAS_DMA (1)
> #else
> #define VIRTIO_CONSOLE_HAS_DMA (0)
> #endif
OK, good point. Perhaps we could even exploit gcc's
ability to remove dead code and do something like this:
if (VIRTIO_CONSOLE_HAS_DMA &&
virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
...
This does not give any linker warnings when compiling for UML (no DMA).
Regards,
Sjur
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael,
If an architecture do not support DMA you will get
a link error: unknown symbol for dma_alloc_coherent.
Yes, it even seems intentional.
But I have a question: can the device work without DMA?
The main dependency is actually on the dma-allocation.
In my case I do dma_declare_coherent_memory() with
the memory area shared with the remote device as argument.
Subsequent calls to dma_alloc_coherent() will then allocate
from this memory area.
Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
Yes.dma_alloc_coherent to work.
If yes you should just fail load.
Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding
VIRTIO_CONSOLE_F_DMA_MEM to the feature array.
Also let's add a wrapper at top of file so ifdefs
do not litter the code like this. For example:
#ifdef CONFIG_HAS_DMA
#define VIRTIO_CONSOLE_HAS_DMA (1)
#else
#define VIRTIO_CONSOLE_HAS_DMA (0)
#endif
OK, good point. Perhaps we could even exploit gcc's
ability to remove dead code and do something like this:
if (VIRTIO_CONSOLE_HAS_DMA
virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
...
This does not give any linker warnings when compiling for UML (no DMA).
Regards,
Sjur
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Tue, Sep 04, 2012 at 01:28:36PM +0200, Sjur Brændeland wrote:
Hi Michael,
If an architecture do not support DMA you will get
a link error: unknown symbol for dma_alloc_coherent.
Yes, it even seems intentional.
But I have a question: can the device work without DMA?
The main dependency is actually on the dma-allocation.
In my case I do dma_declare_coherent_memory() with
the memory area shared with the remote device as argument.
Subsequent calls to dma_alloc_coherent() will then allocate
from this memory area.
Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
Yes.dma_alloc_coherent to work.
If yes you should just fail load.
Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding
VIRTIO_CONSOLE_F_DMA_MEM to the feature array.
Also let's add a wrapper at top of file so ifdefs
do not litter the code like this. For example:
#ifdef CONFIG_HAS_DMA
#define VIRTIO_CONSOLE_HAS_DMA (1)
#else
#define VIRTIO_CONSOLE_HAS_DMA (0)
#endif
OK, good point. Perhaps we could even exploit gcc's
ability to remove dead code and do something like this:
if (VIRTIO_CONSOLE_HAS_DMA
virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
...
This does not give any linker warnings when compiling for UML (no DMA).
Regards,
Sjur
Exactly. Though if we just fail load it will be much less code.
Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.
--
MST
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael, Exactly. Though if we just fail load it will be much less code. Generally, using a feature bit for this is a bit of a problem though: normally driver is expected to be able to simply ignore a feature bit. In this case driver is required to do something so a feature bit is not a good fit. I am not sure what the right thing to do is. I see - so in order to avoid the binding between driver and device there are two options I guess. Either make virtio_dev_match() or virtcons_probe() fail. Neither of them seems like the obvious choice. Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match between device and driver in virtcons_probe() is the lesser evil? Regards, Sjur -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: Hi Michael, Exactly. Though if we just fail load it will be much less code. Generally, using a feature bit for this is a bit of a problem though: normally driver is expected to be able to simply ignore a feature bit. In this case driver is required to do something so a feature bit is not a good fit. I am not sure what the right thing to do is. I see - so in order to avoid the binding between driver and device there are two options I guess. Either make virtio_dev_match() or virtcons_probe() fail. Neither of them seems like the obvious choice. Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match between device and driver in virtcons_probe() is the lesser evil? Regards, Sjur A simplest thing to do is change dev id. rusty? -- MST -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Michael S. Tsirkin m...@redhat.com writes: Also let's add a wrapper at top of file so ifdefs do not litter the code like this. For example: #ifdef CONFIG_HAS_DMA #define VIRTIO_CONSOLE_HAS_DMA (1) #else #define VIRTIO_CONSOLE_HAS_DMA (0) #endif Now use if instead of ifdef. The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you don't have DMA! Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Mon, Sep 03, 2012 at 04:57:45PM +0200, Sjur Brændeland wrote:
> Hi Michael,
>
> > How does access to descriptors work in this setup?
>
> When the ring is setup by remoteproc the descriptors are
> also allocated using dma_alloc_coherent().
>
> >> -static void free_buf(struct port_buffer *buf)
> >> +/* Allcoate data buffer from DMA memory if requested */
> >
> > typo
>
> Thanks.
>
> >> +static inline void *
> >> +alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t
> >> *dma_handle,
> >> +gfp_t flag)
> >> {
> >> - kfree(buf->buf);
> >> +#ifdef CONFIG_HAS_DMA
> >> + if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
> >> + struct device *dev = >dev;
> >> + /*
> >> + * Allocate DMA memory from ancestors. Finding the ancestor
> >> + * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
> >> + * implemented.
> >> + */
> >> + dev = dev->parent ? dev->parent : dev;
> >> + dev = dev->parent ? dev->parent : dev;
> >> + return dma_alloc_coherent(dev, size, dma_handle, flag);
> >> + }
> >> +#endif
> >
> > Are these ifdefs really needed? If DMA_MEM is set,
> > can't we use dma_alloc_coherent
> > unconditionally?
>
> If an architecture do not support DMA you will get
> a link error: "unknown symbol" for dma_alloc_coherent.
>
> Regards,
> Sjur
Yes, it even seems intentional.
But I have a question: can the device work without DMA?
Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
If yes you should just fail load.
Also let's add a wrapper at top of file so ifdefs
do not litter the code like this. For example:
#ifdef CONFIG_HAS_DMA
#define VIRTIO_CONSOLE_HAS_DMA (1)
#else
#define VIRTIO_CONSOLE_HAS_DMA (0)
#endif
Now use if instead of ifdef.
--
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael,
> How does access to descriptors work in this setup?
When the ring is setup by remoteproc the descriptors are
also allocated using dma_alloc_coherent().
>> -static void free_buf(struct port_buffer *buf)
>> +/* Allcoate data buffer from DMA memory if requested */
>
> typo
Thanks.
>> +static inline void *
>> +alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t
>> *dma_handle,
>> +gfp_t flag)
>> {
>> - kfree(buf->buf);
>> +#ifdef CONFIG_HAS_DMA
>> + if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
>> + struct device *dev = >dev;
>> + /*
>> + * Allocate DMA memory from ancestors. Finding the ancestor
>> + * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
>> + * implemented.
>> + */
>> + dev = dev->parent ? dev->parent : dev;
>> + dev = dev->parent ? dev->parent : dev;
>> + return dma_alloc_coherent(dev, size, dma_handle, flag);
>> + }
>> +#endif
>
> Are these ifdefs really needed? If DMA_MEM is set,
> can't we use dma_alloc_coherent
> unconditionally?
If an architecture do not support DMA you will get
a link error: "unknown symbol" for dma_alloc_coherent.
Regards,
Sjur
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Mon, Sep 03, 2012 at 03:51:16PM +0200, sjur.brandel...@stericsson.com wrote:
> From: Sjur Brændeland
>
> Add feature VIRTIO_CONSOLE_F_DMA_MEM. If the architecture has
> DMA support and this feature bit is set, the virtio data buffers
> will be allocated from DMA memory.
>
> This is needed for using virtio_console from the remoteproc
> framework.
>
> Signed-off-by: Sjur Brændeland
> cc: Rusty Russell
> cc: Michael S. Tsirkin
> cc: Ohad Ben-Cohen
> cc: Linus Walleij
> cc: virtualizat...@lists.linux-foundation.org
How does access to descriptors work in this setup?
> ---
> drivers/char/virtio_console.c | 76
>
> include/linux/virtio_console.h |1 +
> 2 files changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index cdf2f54..6bfbd09 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -35,6 +35,7 @@
> #include
> #include
> #include
> +#include
> #include "../tty/hvc/hvc_console.h"
>
> /*
> @@ -334,20 +335,60 @@ static inline bool use_multiport(struct ports_device
> *portdev)
> return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
> }
>
> -static void free_buf(struct port_buffer *buf)
> +/* Allcoate data buffer from DMA memory if requested */
typo
> +static inline void *
> +alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t
> *dma_handle,
> +gfp_t flag)
> {
> - kfree(buf->buf);
> +#ifdef CONFIG_HAS_DMA
> + if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
> + struct device *dev = >dev;
> + /*
> + * Allocate DMA memory from ancestors. Finding the ancestor
> + * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
> + * implemented.
> + */
> + dev = dev->parent ? dev->parent : dev;
> + dev = dev->parent ? dev->parent : dev;
> + return dma_alloc_coherent(dev, size, dma_handle, flag);
> + }
> +#endif
Are these ifdefs really needed? If DMA_MEM is set,
can't we use dma_alloc_coherent
unconditionally?
> + return kzalloc(size, flag);
> +}
> +
> +static inline void
> +free_databuf(struct virtio_device *vdev, size_t size, void *cpu_addr)
> +{
> +#ifdef CONFIG_HAS_DMA
> + dma_addr_t dma_handle = virt_to_bus(cpu_addr);
> + if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
> + struct device *dev = >dev;
> +
> + dev = dev->parent ? dev->parent : dev;
> + dev = dev->parent ? dev->parent : dev;
> + dma_free_coherent(dev, size, cpu_addr, dma_handle);
> + return;
> + }
> +#endif
> + kfree(cpu_addr);
> +}
> +
> +static void
> +free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
> +{
> + free_databuf(vq->vdev, buf_size, buf);
> kfree(buf);
> }
>
> -static struct port_buffer *alloc_buf(size_t buf_size)
> +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size)
> {
> struct port_buffer *buf;
> + dma_addr_t dma;
>
> buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> if (!buf)
> goto fail;
> - buf->buf = kzalloc(buf_size, GFP_KERNEL);
> + buf->buf = alloc_databuf(vq->vdev, buf_size, , GFP_KERNEL);
> if (!buf->buf)
> goto free_buf;
> buf->len = 0;
> @@ -414,7 +455,7 @@ static void discard_port_data(struct port *port)
> port->stats.bytes_discarded += buf->len - buf->offset;
> if (add_inbuf(port->in_vq, buf) < 0) {
> err++;
> - free_buf(buf);
> + free_buf(port->in_vq, buf, PAGE_SIZE);
> }
> port->inbuf = NULL;
> buf = get_inbuf(port);
> @@ -485,7 +526,7 @@ static void reclaim_consumed_buffers(struct port *port)
> return;
> }
> while ((buf = virtqueue_get_buf(port->out_vq, ))) {
> - kfree(buf);
> + free_databuf(port->portdev->vdev, len, buf);
> port->outvq_full = false;
> }
> }
> @@ -672,6 +713,8 @@ static ssize_t port_fops_write(struct file *filp, const
> char __user *ubuf,
> char *buf;
> ssize_t ret;
> bool nonblock;
> + struct virtio_device *vdev;
> + dma_addr_t dma;
>
> /* Userspace could be out to fool us */
> if (!count)
> @@ -694,9 +737,10 @@ static ssize_t port_fops_write(struct file *filp, const
> char __user *ubuf,
> if (!port->guest_connected)
> return -ENODEV;
>
> + vdev = port->portdev->vdev;
> count = min((size_t)(32 * 1024), count);
>
> - buf = kmalloc(count, GFP_KERNEL);
> + buf = alloc_databuf(vdev, count, , GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
>
> @@ -720,7 +764,8 @@ static ssize_t port_fops_write(struct file *filp, const
> char
[RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Sjur Brændeland
Add feature VIRTIO_CONSOLE_F_DMA_MEM. If the architecture has
DMA support and this feature bit is set, the virtio data buffers
will be allocated from DMA memory.
This is needed for using virtio_console from the remoteproc
framework.
Signed-off-by: Sjur Brændeland
cc: Rusty Russell
cc: Michael S. Tsirkin
cc: Ohad Ben-Cohen
cc: Linus Walleij
cc: virtualizat...@lists.linux-foundation.org
---
drivers/char/virtio_console.c | 76
include/linux/virtio_console.h |1 +
2 files changed, 62 insertions(+), 15 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index cdf2f54..6bfbd09 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#include "../tty/hvc/hvc_console.h"
/*
@@ -334,20 +335,60 @@ static inline bool use_multiport(struct ports_device
*portdev)
return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
}
-static void free_buf(struct port_buffer *buf)
+/* Allcoate data buffer from DMA memory if requested */
+static inline void *
+alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t *dma_handle,
+ gfp_t flag)
{
- kfree(buf->buf);
+#ifdef CONFIG_HAS_DMA
+ if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+ struct device *dev = >dev;
+ /*
+* Allocate DMA memory from ancestors. Finding the ancestor
+* is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
+* implemented.
+*/
+ dev = dev->parent ? dev->parent : dev;
+ dev = dev->parent ? dev->parent : dev;
+ return dma_alloc_coherent(dev, size, dma_handle, flag);
+ }
+#endif
+ return kzalloc(size, flag);
+}
+
+static inline void
+free_databuf(struct virtio_device *vdev, size_t size, void *cpu_addr)
+{
+#ifdef CONFIG_HAS_DMA
+ dma_addr_t dma_handle = virt_to_bus(cpu_addr);
+ if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+ struct device *dev = >dev;
+
+ dev = dev->parent ? dev->parent : dev;
+ dev = dev->parent ? dev->parent : dev;
+ dma_free_coherent(dev, size, cpu_addr, dma_handle);
+ return;
+ }
+#endif
+ kfree(cpu_addr);
+}
+
+static void
+free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
+{
+ free_databuf(vq->vdev, buf_size, buf);
kfree(buf);
}
-static struct port_buffer *alloc_buf(size_t buf_size)
+static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size)
{
struct port_buffer *buf;
+ dma_addr_t dma;
buf = kmalloc(sizeof(*buf), GFP_KERNEL);
if (!buf)
goto fail;
- buf->buf = kzalloc(buf_size, GFP_KERNEL);
+ buf->buf = alloc_databuf(vq->vdev, buf_size, , GFP_KERNEL);
if (!buf->buf)
goto free_buf;
buf->len = 0;
@@ -414,7 +455,7 @@ static void discard_port_data(struct port *port)
port->stats.bytes_discarded += buf->len - buf->offset;
if (add_inbuf(port->in_vq, buf) < 0) {
err++;
- free_buf(buf);
+ free_buf(port->in_vq, buf, PAGE_SIZE);
}
port->inbuf = NULL;
buf = get_inbuf(port);
@@ -485,7 +526,7 @@ static void reclaim_consumed_buffers(struct port *port)
return;
}
while ((buf = virtqueue_get_buf(port->out_vq, ))) {
- kfree(buf);
+ free_databuf(port->portdev->vdev, len, buf);
port->outvq_full = false;
}
}
@@ -672,6 +713,8 @@ static ssize_t port_fops_write(struct file *filp, const
char __user *ubuf,
char *buf;
ssize_t ret;
bool nonblock;
+ struct virtio_device *vdev;
+ dma_addr_t dma;
/* Userspace could be out to fool us */
if (!count)
@@ -694,9 +737,10 @@ static ssize_t port_fops_write(struct file *filp, const
char __user *ubuf,
if (!port->guest_connected)
return -ENODEV;
+ vdev = port->portdev->vdev;
count = min((size_t)(32 * 1024), count);
- buf = kmalloc(count, GFP_KERNEL);
+ buf = alloc_databuf(vdev, count, , GFP_KERNEL);
if (!buf)
return -ENOMEM;
@@ -720,7 +764,8 @@ static ssize_t port_fops_write(struct file *filp, const
char __user *ubuf,
goto out;
free_buf:
- kfree(buf);
+ free_databuf(vdev, count, buf);
+
out:
return ret;
}
@@ -1102,7 +1147,7 @@ static unsigned int fill_queue(struct virtqueue *vq,
spinlock_t *lock)
nr_added_bufs = 0;
do {
- buf = alloc_buf(PAGE_SIZE);
+ buf = alloc_buf(vq, PAGE_SIZE);
if (!buf)
[RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Sjur Brændeland sjur.brandel...@stericsson.com
Add feature VIRTIO_CONSOLE_F_DMA_MEM. If the architecture has
DMA support and this feature bit is set, the virtio data buffers
will be allocated from DMA memory.
This is needed for using virtio_console from the remoteproc
framework.
Signed-off-by: Sjur Brændeland sjur.brandel...@stericsson.com
cc: Rusty Russell ru...@rustcorp.com.au
cc: Michael S. Tsirkin m...@redhat.com
cc: Ohad Ben-Cohen o...@wizery.com
cc: Linus Walleij linus.wall...@linaro.org
cc: virtualizat...@lists.linux-foundation.org
---
drivers/char/virtio_console.c | 76
include/linux/virtio_console.h |1 +
2 files changed, 62 insertions(+), 15 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index cdf2f54..6bfbd09 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -35,6 +35,7 @@
#include linux/wait.h
#include linux/workqueue.h
#include linux/module.h
+#include linux/dma-mapping.h
#include ../tty/hvc/hvc_console.h
/*
@@ -334,20 +335,60 @@ static inline bool use_multiport(struct ports_device
*portdev)
return portdev-vdev-features[0] (1 VIRTIO_CONSOLE_F_MULTIPORT);
}
-static void free_buf(struct port_buffer *buf)
+/* Allcoate data buffer from DMA memory if requested */
+static inline void *
+alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t *dma_handle,
+ gfp_t flag)
{
- kfree(buf-buf);
+#ifdef CONFIG_HAS_DMA
+ if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+ struct device *dev = vdev-dev;
+ /*
+* Allocate DMA memory from ancestors. Finding the ancestor
+* is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
+* implemented.
+*/
+ dev = dev-parent ? dev-parent : dev;
+ dev = dev-parent ? dev-parent : dev;
+ return dma_alloc_coherent(dev, size, dma_handle, flag);
+ }
+#endif
+ return kzalloc(size, flag);
+}
+
+static inline void
+free_databuf(struct virtio_device *vdev, size_t size, void *cpu_addr)
+{
+#ifdef CONFIG_HAS_DMA
+ dma_addr_t dma_handle = virt_to_bus(cpu_addr);
+ if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+ struct device *dev = vdev-dev;
+
+ dev = dev-parent ? dev-parent : dev;
+ dev = dev-parent ? dev-parent : dev;
+ dma_free_coherent(dev, size, cpu_addr, dma_handle);
+ return;
+ }
+#endif
+ kfree(cpu_addr);
+}
+
+static void
+free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
+{
+ free_databuf(vq-vdev, buf_size, buf);
kfree(buf);
}
-static struct port_buffer *alloc_buf(size_t buf_size)
+static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size)
{
struct port_buffer *buf;
+ dma_addr_t dma;
buf = kmalloc(sizeof(*buf), GFP_KERNEL);
if (!buf)
goto fail;
- buf-buf = kzalloc(buf_size, GFP_KERNEL);
+ buf-buf = alloc_databuf(vq-vdev, buf_size, dma, GFP_KERNEL);
if (!buf-buf)
goto free_buf;
buf-len = 0;
@@ -414,7 +455,7 @@ static void discard_port_data(struct port *port)
port-stats.bytes_discarded += buf-len - buf-offset;
if (add_inbuf(port-in_vq, buf) 0) {
err++;
- free_buf(buf);
+ free_buf(port-in_vq, buf, PAGE_SIZE);
}
port-inbuf = NULL;
buf = get_inbuf(port);
@@ -485,7 +526,7 @@ static void reclaim_consumed_buffers(struct port *port)
return;
}
while ((buf = virtqueue_get_buf(port-out_vq, len))) {
- kfree(buf);
+ free_databuf(port-portdev-vdev, len, buf);
port-outvq_full = false;
}
}
@@ -672,6 +713,8 @@ static ssize_t port_fops_write(struct file *filp, const
char __user *ubuf,
char *buf;
ssize_t ret;
bool nonblock;
+ struct virtio_device *vdev;
+ dma_addr_t dma;
/* Userspace could be out to fool us */
if (!count)
@@ -694,9 +737,10 @@ static ssize_t port_fops_write(struct file *filp, const
char __user *ubuf,
if (!port-guest_connected)
return -ENODEV;
+ vdev = port-portdev-vdev;
count = min((size_t)(32 * 1024), count);
- buf = kmalloc(count, GFP_KERNEL);
+ buf = alloc_databuf(vdev, count, dma, GFP_KERNEL);
if (!buf)
return -ENOMEM;
@@ -720,7 +764,8 @@ static ssize_t port_fops_write(struct file *filp, const
char __user *ubuf,
goto out;
free_buf:
- kfree(buf);
+ free_databuf(vdev, count, buf);
+
out:
return ret;
}
@@ -1102,7 +1147,7 @@ static unsigned int fill_queue(struct virtqueue *vq,
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Mon, Sep 03, 2012 at 03:51:16PM +0200, sjur.brandel...@stericsson.com wrote:
From: Sjur Brændeland sjur.brandel...@stericsson.com
Add feature VIRTIO_CONSOLE_F_DMA_MEM. If the architecture has
DMA support and this feature bit is set, the virtio data buffers
will be allocated from DMA memory.
This is needed for using virtio_console from the remoteproc
framework.
Signed-off-by: Sjur Brændeland sjur.brandel...@stericsson.com
cc: Rusty Russell ru...@rustcorp.com.au
cc: Michael S. Tsirkin m...@redhat.com
cc: Ohad Ben-Cohen o...@wizery.com
cc: Linus Walleij linus.wall...@linaro.org
cc: virtualizat...@lists.linux-foundation.org
How does access to descriptors work in this setup?
---
drivers/char/virtio_console.c | 76
include/linux/virtio_console.h |1 +
2 files changed, 62 insertions(+), 15 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index cdf2f54..6bfbd09 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -35,6 +35,7 @@
#include linux/wait.h
#include linux/workqueue.h
#include linux/module.h
+#include linux/dma-mapping.h
#include ../tty/hvc/hvc_console.h
/*
@@ -334,20 +335,60 @@ static inline bool use_multiport(struct ports_device
*portdev)
return portdev-vdev-features[0] (1 VIRTIO_CONSOLE_F_MULTIPORT);
}
-static void free_buf(struct port_buffer *buf)
+/* Allcoate data buffer from DMA memory if requested */
typo
+static inline void *
+alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t
*dma_handle,
+gfp_t flag)
{
- kfree(buf-buf);
+#ifdef CONFIG_HAS_DMA
+ if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+ struct device *dev = vdev-dev;
+ /*
+ * Allocate DMA memory from ancestors. Finding the ancestor
+ * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
+ * implemented.
+ */
+ dev = dev-parent ? dev-parent : dev;
+ dev = dev-parent ? dev-parent : dev;
+ return dma_alloc_coherent(dev, size, dma_handle, flag);
+ }
+#endif
Are these ifdefs really needed? If DMA_MEM is set,
can't we use dma_alloc_coherent
unconditionally?
+ return kzalloc(size, flag);
+}
+
+static inline void
+free_databuf(struct virtio_device *vdev, size_t size, void *cpu_addr)
+{
+#ifdef CONFIG_HAS_DMA
+ dma_addr_t dma_handle = virt_to_bus(cpu_addr);
+ if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+ struct device *dev = vdev-dev;
+
+ dev = dev-parent ? dev-parent : dev;
+ dev = dev-parent ? dev-parent : dev;
+ dma_free_coherent(dev, size, cpu_addr, dma_handle);
+ return;
+ }
+#endif
+ kfree(cpu_addr);
+}
+
+static void
+free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
+{
+ free_databuf(vq-vdev, buf_size, buf);
kfree(buf);
}
-static struct port_buffer *alloc_buf(size_t buf_size)
+static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size)
{
struct port_buffer *buf;
+ dma_addr_t dma;
buf = kmalloc(sizeof(*buf), GFP_KERNEL);
if (!buf)
goto fail;
- buf-buf = kzalloc(buf_size, GFP_KERNEL);
+ buf-buf = alloc_databuf(vq-vdev, buf_size, dma, GFP_KERNEL);
if (!buf-buf)
goto free_buf;
buf-len = 0;
@@ -414,7 +455,7 @@ static void discard_port_data(struct port *port)
port-stats.bytes_discarded += buf-len - buf-offset;
if (add_inbuf(port-in_vq, buf) 0) {
err++;
- free_buf(buf);
+ free_buf(port-in_vq, buf, PAGE_SIZE);
}
port-inbuf = NULL;
buf = get_inbuf(port);
@@ -485,7 +526,7 @@ static void reclaim_consumed_buffers(struct port *port)
return;
}
while ((buf = virtqueue_get_buf(port-out_vq, len))) {
- kfree(buf);
+ free_databuf(port-portdev-vdev, len, buf);
port-outvq_full = false;
}
}
@@ -672,6 +713,8 @@ static ssize_t port_fops_write(struct file *filp, const
char __user *ubuf,
char *buf;
ssize_t ret;
bool nonblock;
+ struct virtio_device *vdev;
+ dma_addr_t dma;
/* Userspace could be out to fool us */
if (!count)
@@ -694,9 +737,10 @@ static ssize_t port_fops_write(struct file *filp, const
char __user *ubuf,
if (!port-guest_connected)
return -ENODEV;
+ vdev = port-portdev-vdev;
count = min((size_t)(32 * 1024), count);
- buf = kmalloc(count, GFP_KERNEL);
+ buf = alloc_databuf(vdev, count, dma, GFP_KERNEL);
if (!buf)
return -ENOMEM;
@@ -720,7 +764,8 @@ static ssize_t
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael,
How does access to descriptors work in this setup?
When the ring is setup by remoteproc the descriptors are
also allocated using dma_alloc_coherent().
-static void free_buf(struct port_buffer *buf)
+/* Allcoate data buffer from DMA memory if requested */
typo
Thanks.
+static inline void *
+alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t
*dma_handle,
+gfp_t flag)
{
- kfree(buf-buf);
+#ifdef CONFIG_HAS_DMA
+ if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+ struct device *dev = vdev-dev;
+ /*
+ * Allocate DMA memory from ancestors. Finding the ancestor
+ * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
+ * implemented.
+ */
+ dev = dev-parent ? dev-parent : dev;
+ dev = dev-parent ? dev-parent : dev;
+ return dma_alloc_coherent(dev, size, dma_handle, flag);
+ }
+#endif
Are these ifdefs really needed? If DMA_MEM is set,
can't we use dma_alloc_coherent
unconditionally?
If an architecture do not support DMA you will get
a link error: unknown symbol for dma_alloc_coherent.
Regards,
Sjur
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
On Mon, Sep 03, 2012 at 04:57:45PM +0200, Sjur Brændeland wrote:
Hi Michael,
How does access to descriptors work in this setup?
When the ring is setup by remoteproc the descriptors are
also allocated using dma_alloc_coherent().
-static void free_buf(struct port_buffer *buf)
+/* Allcoate data buffer from DMA memory if requested */
typo
Thanks.
+static inline void *
+alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t
*dma_handle,
+gfp_t flag)
{
- kfree(buf-buf);
+#ifdef CONFIG_HAS_DMA
+ if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+ struct device *dev = vdev-dev;
+ /*
+ * Allocate DMA memory from ancestors. Finding the ancestor
+ * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
+ * implemented.
+ */
+ dev = dev-parent ? dev-parent : dev;
+ dev = dev-parent ? dev-parent : dev;
+ return dma_alloc_coherent(dev, size, dma_handle, flag);
+ }
+#endif
Are these ifdefs really needed? If DMA_MEM is set,
can't we use dma_alloc_coherent
unconditionally?
If an architecture do not support DMA you will get
a link error: unknown symbol for dma_alloc_coherent.
Regards,
Sjur
Yes, it even seems intentional.
But I have a question: can the device work without DMA?
Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
If yes you should just fail load.
Also let's add a wrapper at top of file so ifdefs
do not litter the code like this. For example:
#ifdef CONFIG_HAS_DMA
#define VIRTIO_CONSOLE_HAS_DMA (1)
#else
#define VIRTIO_CONSOLE_HAS_DMA (0)
#endif
Now use if instead of ifdef.
--
MST
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
